Waiting for review: https://github.com/aibasel/downward/pull/244

Thanks, David! 

As expected, we lose a bit of time during M&S construction due to considering all partitions instead of only one at a time. The other scores look ok to me - we lose a bit of performance with scc-sbmiasm but not really with scc-dfp. What do you think, Malte?

Here are the new experiments: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1171/data/issue1171-v4-eval/

Oups, then I must accidentally have used base-v3 instead of base-v2, so please use that one.

@Silvan No worries. Did you push the issue1171-base-v2 tag/branch? I can only see issue1171-base-v3 as a tag (https://github.com/silvansievers/downward/tags).

@David: issue1171-base-v2 is *one* revision that I used for the main branch the issue branch is based on. so issue1171-base, which you used, is probably 6-9 months old, from when we last ran the experiments. Sorry for the confusing naming! If possible, I think it would be cleaner to re-run with the correct baseline. (Altough the results look good to me, already, but I don't know how much main changed in the last half year or more.)

I reran the experiments (including the domain from IPC 2023), comparing issue1171-base and issue1171-v3: https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1171/data/issue1171-v3-eval/

@Silvan: I just noticed that you mentioned issue1171-base-v2. If you actually wanted to compare v2 to v3, I can of course rerun the experiments.

Thanks, Silvan! The commit looks good to me. If we're happy with the experimental results, from my side we can merge.

Thanks! I made the changes and rebased onto the latest main.

@David: could you please re-run the experiments using revisions issue1171-base-v2 and issue1171-v3?

I have fixed the current report nevertheless.

Sure. Ping me when I should rerun the experiments and create some meaningful diff reports :^).

Ok, I'll look into using remove_if - @David, no need to adapt experiment reports just yet, I think we'll have to re-run them again :)

Regarding the note: happy to not have it.

No, you are right that the comparison report is wrong. The script doesn't take into account the revisions. 

@David: could you please fix this? The comparison report should compare
[('issue1171-base-b50k-t900-sccdfp', 'issue1171-v2-b50k-t900-sccdfp'), ('issue1171-base-b50k-t900-sccsbmiasm', 'issue1171-v2-b50k-t900-sccsbmiasm')]

Probably changing the two lines could be enough (untested, writing from the wrong computer):
algo_nicks_old = [f'{REVISIONS[0]}-{config.nick}' for config in CONFIGS]
algo_nicks_new = [f'{REVISIONS[1]}-{config.nick}' for config in CONFIGS]

> @Malte @Gabi are you up for a review? Malte, you already reviewed it a while
> ago, so it would probably make sense if you could have a another look.

I hope I looked at the right code -- the one I looked at still had a comment where I asked for a change without a reaction. I've added a new reply to that, plus one more new comment.

Apart from these comments, the new version looks fine to me regarding the things I mentioned in the first review (code complexity and option complexity). I don't have time to go into the semantics of the change, i.e., really judge what the code actually does. But if you're happy with that, that's good enough for me.

So once the two pull request comments and the question on the experiments are resolved, it looks good to merge for me.

> As with v1, the experiments show no significant changes, if anything, a minor
> slow-down which is expected given that the score-based merge strategy has
> more options to consider in each iteration.

Am I looking at the wrong thing?

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1171/data/issue1171-v2-eval/issue1171-v2-compare.html

seems to compare a bunch of configurations to themselves (turns out the deltas are all 0.00 then).

I can look at the absolute numbers rather than the comparative, but the presence of this comparison has me confused, and I'd rather look at comparative numbers if possible.

@Malte @Gabi are you up for a review? Malte, you already reviewed it a while ago, so it would probably make sense if you could have a another look.

Yes, thanks! No worries about the benchmarks ;) 

As with v1, the experiments show no significant changes, if anything, a minor slow-down which is expected given that the score-based merge strategy has more options to consider in each iteration.

Since I changed the code after the first review, I think this needs another round before we can possibly conclude.

I ran the experiments with the new configurations both versions (base,v2): https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1171/

Was this what you had in mind? 

The benchmark set is still the one without the domains from the new IPC 2023. I forgot to update it, although you actually mentioned it :^). However, I hope the experiments are conclusive enough. If not, I have now set everything up again. It should be relatively easy to run new versions.

I rebased the branch and, in a second commit, removed the newly introduced option and the "order of SCCs" option as discussed. The patch now simplifies the code and removes an option. Please review.

Could someone re-run the experiments we did last year? Looking at you, David ;-)

The v1.py script that David ran last time needs to use updated configs:

for strat_name, strat_cmd in [
    ('sccdfp', 'scoring_functions=[goal_relevance(),dfp(),total_order(atomic_ts_order=reverse_level,product_ts_order=new_to_old,atomic_before_product=false)]'),
    ('sccsbmiasm', 'scoring_functions=[sf_miasm(shrink_strategy=shrink_bisimulation(greedy=false),max_states=50000,threshold_before_merge=1),total_order(atomic_ts_order=reverse_level,product_ts_order=new_to_old,atomic_before_product=false,random_seed=2016)]')]:
    CONFIGS.append(IssueConfig(f'b50k-t900-{strat_name}', ['--search', f'astar(merge_and_shrink(shrink_strategy=shrink_bisimulation(greedy=false),label_reduction=exact(before_shrinking=true,before_merging=false),merge_strategy=merge_sccs(merge_selector=score_based_filtering({strat_cmd})),max_states=50K,threshold_before_merge=1,main_loop_max_time=900),verbosity=silent)']))

Also, the script should now run both on the base version (issue1171-base) and v2 (issue1171-v2). I didn't adapt this because I assume that the script might need to be further adapted to cater for a new version of Lab or changes on the cluster, or the benchmarks etc.

In case you're still waiting for an answer: sounds good to me.

I agree regarding having fewer options is better. Now that we talked about this, another alternative could be to not introduce the new option, but permanently change the behavior. Doing so would even allow to remove the old option for specifying the order in which each SCC should be dealt with. I never used that option outside the experiments of the paper introducing the strategy. If we go with that alternative, we should document the changed behavior at the existing SCC merge strategy. 

I think I actually slightly prefer this alternative. What do you think?

I wondered about the reason why we want to merge it. It's clearer to me after your answer.

To explain my thought process:

The codebase becomes larger, and now we have more different options to choose from. That's a negative from the complexity standpoint, so we need a positive to offset it.

That you think the new way is more natural would be a stronger argument if it replaced the only way, but here we're adding an option and now have to maintain both ways in the future.

If the new options are useful for something that we will merge later, that's a good reason to include the new option. Adding the reference to the paper that uses the option is an important aspect of this for me, but of course it's fine to add this reference later when it makes more sense, in the place where it makes sense.

Is the place for the reference that you intended the user documentation for the new scoring function(s)? That sounds good to me.

> You write "We used this option for combining the  "maximum factor"/"maximum SCP" scoring functions from the ICAPS 2024 paper (https://ai.dmi.unibas.ch/papers/sievers-et-al-icaps2024.pdf) with the SCC merge strategy". Who is "we" and where did you use it? In that paper itself?

Yes, "we" was meant as "the authors of that paper". For the paper, we changed the SCC strategy to allow working on any SCC partition because that was a requirement for the "filter-based merge strategy", which would stop the M&S computation once it found that there is no more merge candidate that, when selected, would improve the M&S heuristic compared to stopping the M&S computation. When insisting to consider only one SCC partition at a time, that case would often occur quickly, even though other SCC partitions could potentially offer many more good merge opportunities.

The best configurations in the paper all use the new option. I also think that the new option feels more natural than the old one, but that is probably a question of personal preference.

> If this was used in something somewhere, it would be good to add this to the documentation.
It is used in the paper. I planned to add the reference only when integrating issue1172, where I want to integrate the actual new scoring functions, but I'm happy to include the paper reference already now, if you prefer.

I did a code review.

From the experiments, this doesn't seem to have much of an impact, so I want to ask a bit more about the motivation to test this.

You write "We used this option for combining the  "maximum factor"/"maximum SCP" scoring functions from the ICAPS 2024 paper (https://ai.dmi.unibas.ch/papers/sievers-et-al-icaps2024.pdf) with the SCC merge strategy". Who is "we" and where did you use it? In that paper itself?

If this was used in something somewhere, it would be good to add this to the documentation. Right now it's a bit hard to see the motivation for the change, as every option makes things a little bit more complicated.

Thanks! They look good to me.

@Silvan Here are the results of the experiments:

https://ai.dmi.unibas.ch/_experiments/ai/downward/issue1171/

@Gabi: everything is ready for experiments, except the modifications required for the cluster. Could you please run experiments? Or find some other volunteer? ;)

The pull-request is also ready for being reviewed: https://github.com/aibasel/downward/pull/244

I wanted to run some experiments, since I wanted to make the new option the default for the SCC merge strategy. I started preparing the scripts but I'll look into issue1173 first.

From my understanding (given the PR), is there any experiment planned for this 
issue or has it already been done? We found some issues with the parameter order 
in issue1173 regarding M&S that may affect them.

In the current implementation, the SCCs clusters are "worked on" one after the other, according to a used-specified order. In this issue, I'd like to add an option to allow merging the best candidate according to the used merge selector, where candidates stem from *all* SCC clusters. We used this option for combining the  "maximum factor"/"maximum SCP" scoring functions from the ICAPS 2024 paper (https://ai.dmi.unibas.ch/papers/sievers-et-al-icaps2024.pdf) with the SCC merge strategy.