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

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.