From a17e1c98c7cbc3dbc0d04a44a6728e33e9f9933b Mon Sep 17 00:00:00 2001 From: Silvan Sievers Date: Wed, 29 Jul 2026 21:35:07 +0200 Subject: [PATCH 1/2] [issue1171] M&S SCC merge strategy: new option to allow working on any SCC --- .../merge_strategy_factory_sccs.cc | 40 ++++-- .../merge_strategy_factory_sccs.h | 3 +- .../merge_and_shrink/merge_strategy_sccs.cc | 114 ++++++++++-------- .../merge_and_shrink/merge_strategy_sccs.h | 8 +- 4 files changed, 100 insertions(+), 65 deletions(-) diff --git a/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc b/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc index 2b8f3ae13a..3e3d7fe1ef 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc @@ -33,10 +33,12 @@ static bool compare_sccs_decreasing( MergeStrategyFactorySCCs::MergeStrategyFactorySCCs( const shared_ptr &task, const OrderOfSCCs &order_of_sccs, - const shared_ptr &merge_selector, utils::Verbosity verbosity) + const shared_ptr &merge_selector, bool allow_working_on_all_clusters, + utils::Verbosity verbosity) : MergeStrategyFactory(task, verbosity), order_of_sccs(order_of_sccs), - merge_selector(merge_selector) { + merge_selector(merge_selector), + allow_working_on_all_clusters(allow_working_on_all_clusters) { } unique_ptr MergeStrategyFactorySCCs::compute_merge_strategy( @@ -97,7 +99,8 @@ unique_ptr MergeStrategyFactorySCCs::compute_merge_strategy( } return make_unique( - fts, merge_selector, move(non_singleton_cg_sccs)); + fts, merge_selector, move(non_singleton_cg_sccs), + allow_working_on_all_clusters); } bool MergeStrategyFactorySCCs::requires_init_distances() const { @@ -153,16 +156,32 @@ class MergeStrategyFactorySCCsFeature "In a nutshell, it computes the maximal strongly connected " "components (SCCs) of the causal graph, " "obtaining a partitioning of the task's variables. Every such " - "partition is then merged individually, using the specified fallback " - "merge strategy, considering the SCCs in a configurable order. " - "Afterwards, all resulting composite abstractions are merged to form " - "the final abstraction, again using the specified fallback merge " - "strategy and the configurable order of the SCCs."); - + "partition is then merged individually, using the score-based merge " + "strategy specified via the merge_selector option. If " + "allow_working_on_all_clusters=true, then all pairs of factors in " + "each partition form the set of candidates scored by the score-based " + "merge strategy. Otherwise, SCC partitions are worked on 'one after " + "the other' in the order specified via 'order_of_sccs', and hence " + "only the pairs of factors of the 'current partition' form the set " + "of candidates. In both cases, once all partitions have been merged, " + "the resulting product factors are merged according to the score-" + "based merge strategy."); + document_note( + "Note regarding allow_working_on_all_clusters", + "The option allow_working_on_all_clusters was not introduced in the " + "original paper, hence to obtain exactly the configurations of that paper, " + "set the option to false."); add_option( - "order_of_sccs", "how the SCCs should be ordered", "topological"); + "order_of_sccs", + "how the SCCs should be ordered (only relevant if allow_working_on_all_clusters=false)", + "topological"); add_option>( "merge_selector", "the fallback merge strategy to use"); + add_option( + "allow_working_on_all_clusters", + "if true, consider as merge candidates the pairs of factors of all SCCs. If " + "false, fully finish dealing with one cluster at a time.", + "true"); add_merge_strategy_options_to_feature(*this); } @@ -173,6 +192,7 @@ class MergeStrategyFactorySCCsFeature opts.get("order_of_sccs"), opts.get>( "merge_selector"), + opts.get("allow_working_on_all_clusters"), get_merge_strategy_arguments_from_options(opts)); } }; diff --git a/src/search/merge_and_shrink/merge_strategy_factory_sccs.h b/src/search/merge_and_shrink/merge_strategy_factory_sccs.h index 70b39a3bbe..2d0c0c016a 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.h +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.h @@ -16,6 +16,7 @@ enum class OrderOfSCCs { class MergeStrategyFactorySCCs : public MergeStrategyFactory { OrderOfSCCs order_of_sccs; std::shared_ptr merge_selector; + bool allow_working_on_all_clusters; protected: virtual std::string name() const override; virtual void dump_strategy_specific_options() const override; @@ -24,7 +25,7 @@ class MergeStrategyFactorySCCs : public MergeStrategyFactory { const std::shared_ptr &task, const OrderOfSCCs &order_of_sccs, const std::shared_ptr &merge_selector, - utils::Verbosity verbosity); + bool allow_working_on_all_clusters, utils::Verbosity verbosity); virtual std::unique_ptr compute_merge_strategy( const TaskProxy &task_proxy, const FactoredTransitionSystem &fts) override; diff --git a/src/search/merge_and_shrink/merge_strategy_sccs.cc b/src/search/merge_and_shrink/merge_strategy_sccs.cc index 1a8be31fa3..a1d5c6f9db 100644 --- a/src/search/merge_and_shrink/merge_strategy_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_sccs.cc @@ -2,8 +2,6 @@ #include "factored_transition_system.h" #include "merge_selector.h" -#include "merge_tree.h" -#include "merge_tree_factory.h" #include "transition_system.h" #include @@ -16,71 +14,87 @@ namespace merge_and_shrink { MergeStrategySCCs::MergeStrategySCCs( const FactoredTransitionSystem &fts, const shared_ptr &merge_selector, - vector> &&non_singleton_cg_sccs) + vector> &&unfinished_clusters, + bool allow_working_on_all_clusters) : MergeStrategy(fts), merge_selector(merge_selector), - non_singleton_cg_sccs(move(non_singleton_cg_sccs)) { + unfinished_clusters(move(unfinished_clusters)), + allow_working_on_all_clusters(allow_working_on_all_clusters) { } MergeStrategySCCs::~MergeStrategySCCs() { } +static void compute_merge_candidates( + const vector &indices, vector> &merge_candidates) { + for (size_t i = 0; i < indices.size(); ++i) { + int ts_index1 = indices[i]; + for (size_t j = i + 1; j < indices.size(); ++j) { + int ts_index2 = indices[j]; + merge_candidates.emplace_back(ts_index1, ts_index2); + } + } +} + pair MergeStrategySCCs::get_next() { - if (current_ts_indices.empty()) { - /* - We are currently not dealing with merging all factors of an SCC, so - we need to either get the next one or allow merging any existing - factors of the FTS if there is no SCC left. - */ - if (non_singleton_cg_sccs.empty()) { - // We are done dealing with all SCCs, allow merging any factors. - current_ts_indices.reserve(fts.get_num_active_entries()); - for (int ts_index : fts) { - current_ts_indices.push_back(ts_index); + if (unfinished_clusters.empty()) { + // We merged all clusters. + return merge_selector->select_merge(fts); + } else { + // There are clusters we still have to deal with. + vector> merge_candidates; + vector factor_to_cluster; + if (allow_working_on_all_clusters) { + // Compute merge candidate pairs for each cluster. + factor_to_cluster.resize(fts.get_size(), -1); + for (size_t cluster_index = 0; + cluster_index < unfinished_clusters.size(); ++cluster_index) { + const vector &cluster = unfinished_clusters[cluster_index]; + for (int factor : cluster) { + factor_to_cluster[factor] = cluster_index; + } + compute_merge_candidates(cluster, merge_candidates); } } else { - /* - There is another SCC we have to deal with. Store its factors so - that we merge them over the next iterations. - */ - vector ¤t_scc = non_singleton_cg_sccs.front(); - assert(current_scc.size() > 1); - current_ts_indices = move(current_scc); - non_singleton_cg_sccs.erase(non_singleton_cg_sccs.begin()); + // Deal with first cluster. + vector &cluster = unfinished_clusters.front(); + compute_merge_candidates(cluster, merge_candidates); } - } else { - // Add the most recent product to the current index set. - current_ts_indices.push_back(fts.get_size() - 1); - } + // Select the next merge from the allowed merge candidates. + pair next_pair = merge_selector->select_merge_from_candidates( + fts, move(merge_candidates)); - // Compute all merge candidates for the current set of indices. - vector> merge_candidates; - merge_candidates.reserve( - (current_ts_indices.size() * (current_ts_indices.size() - 1)) / 2); - assert(current_ts_indices.size() > 1); - for (size_t i = 0; i < current_ts_indices.size(); ++i) { - int ts_index1 = current_ts_indices[i]; - assert(fts.is_active(ts_index1)); - for (size_t j = i + 1; j < current_ts_indices.size(); ++j) { - int ts_index2 = current_ts_indices[j]; - assert(fts.is_active(ts_index2)); - merge_candidates.emplace_back(ts_index1, ts_index2); + // Get the cluster from which we selected the next merge. + int affected_cluster_index; + if (allow_working_on_all_clusters) { + affected_cluster_index = factor_to_cluster[next_pair.first]; + assert( + affected_cluster_index == factor_to_cluster[next_pair.second]); + } else { + affected_cluster_index = 0; } - } - // Select the next merge for the current set of indices. - pair next_pair = merge_selector->select_merge_from_candidates( - fts, move(merge_candidates)); + // Remove the two merged indices from that cluster. + vector &affected_cluster = + unfinished_clusters[affected_cluster_index]; + for (vector::iterator it = affected_cluster.begin(); + it != affected_cluster.end();) { + if (*it == next_pair.first || *it == next_pair.second) { + it = affected_cluster.erase(it); + } else { + ++it; + } + } - // Remove the two merged indices from the current index set. - for (vector::iterator it = current_ts_indices.begin(); - it != current_ts_indices.end();) { - if (*it == next_pair.first || *it == next_pair.second) { - it = current_ts_indices.erase(it); + if (affected_cluster.empty()) { + // If the cluster got empty, remove it. + unfinished_clusters.erase( + unfinished_clusters.begin() + affected_cluster_index); } else { - ++it; + // Otherwise, add the index of the to-be-created product factor. + affected_cluster.push_back(fts.get_size()); } + return next_pair; } - return next_pair; } } diff --git a/src/search/merge_and_shrink/merge_strategy_sccs.h b/src/search/merge_and_shrink/merge_strategy_sccs.h index 5d25630e03..f79f28710a 100644 --- a/src/search/merge_and_shrink/merge_strategy_sccs.h +++ b/src/search/merge_and_shrink/merge_strategy_sccs.h @@ -10,14 +10,14 @@ namespace merge_and_shrink { class MergeSelector; class MergeStrategySCCs : public MergeStrategy { std::shared_ptr merge_selector; - std::vector> non_singleton_cg_sccs; - - std::vector current_ts_indices; + std::vector> unfinished_clusters; + bool allow_working_on_all_clusters; public: MergeStrategySCCs( const FactoredTransitionSystem &fts, const std::shared_ptr &merge_selector, - std::vector> &&non_singleton_cg_sccs); + std::vector> &&unfinished_clusters, + bool allow_working_on_all_clusters); virtual ~MergeStrategySCCs() override; virtual std::pair get_next() override; }; From e41feb57cfd11d868b6df8a1f6d8f195bdd78926 Mon Sep 17 00:00:00 2001 From: Silvan Sievers Date: Wed, 29 Jul 2026 21:55:43 +0200 Subject: [PATCH 2/2] [issue1171] M&S SCC merge strategy: consider all SCCs at each merge step --- .../merge_strategy_factory_sccs.cc | 104 +++--------------- .../merge_strategy_factory_sccs.h | 12 +- .../merge_and_shrink/merge_strategy_sccs.cc | 39 +++---- .../merge_and_shrink/merge_strategy_sccs.h | 4 +- 4 files changed, 30 insertions(+), 129 deletions(-) diff --git a/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc b/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc index 3e3d7fe1ef..b249027054 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc @@ -21,24 +21,10 @@ using namespace std; namespace merge_and_shrink { -static bool compare_sccs_increasing( - const vector &lhs, const vector &rhs) { - return lhs.size() < rhs.size(); -} - -static bool compare_sccs_decreasing( - const vector &lhs, const vector &rhs) { - return lhs.size() > rhs.size(); -} - MergeStrategyFactorySCCs::MergeStrategyFactorySCCs( - const shared_ptr &task, const OrderOfSCCs &order_of_sccs, - const shared_ptr &merge_selector, bool allow_working_on_all_clusters, - utils::Verbosity verbosity) - : MergeStrategyFactory(task, verbosity), - order_of_sccs(order_of_sccs), - merge_selector(merge_selector), - allow_working_on_all_clusters(allow_working_on_all_clusters) { + const shared_ptr &task, + const shared_ptr &merge_selector, utils::Verbosity verbosity) + : MergeStrategyFactory(task, verbosity), merge_selector(merge_selector) { } unique_ptr MergeStrategyFactorySCCs::compute_merge_strategy( @@ -56,23 +42,6 @@ unique_ptr MergeStrategyFactorySCCs::compute_merge_strategy( } vector> sccs(sccs::compute_maximal_sccs(cg)); - // Put the SCCs in the desired order. - switch (order_of_sccs) { - case OrderOfSCCs::TOPOLOGICAL: - // SCCs are computed in topological order. - break; - case OrderOfSCCs::REVERSE_TOPOLOGICAL: - // SCCs are computed in topological order. - reverse(sccs.begin(), sccs.end()); - break; - case OrderOfSCCs::DECREASING: - sort(sccs.begin(), sccs.end(), compare_sccs_decreasing); - break; - case OrderOfSCCs::INCREASING: - sort(sccs.begin(), sccs.end(), compare_sccs_increasing); - break; - } - if (log.is_at_least_normal()) { log << "SCCs of the causal graph:" << endl; } @@ -99,8 +68,7 @@ unique_ptr MergeStrategyFactorySCCs::compute_merge_strategy( } return make_unique( - fts, merge_selector, move(non_singleton_cg_sccs), - allow_working_on_all_clusters); + fts, merge_selector, move(non_singleton_cg_sccs)); } bool MergeStrategyFactorySCCs::requires_init_distances() const { @@ -113,24 +81,8 @@ bool MergeStrategyFactorySCCs::requires_goal_distances() const { void MergeStrategyFactorySCCs::dump_strategy_specific_options() const { if (log.is_at_least_normal()) { - log << "Merge order of sccs: "; - switch (order_of_sccs) { - case OrderOfSCCs::TOPOLOGICAL: - log << "topological"; - break; - case OrderOfSCCs::REVERSE_TOPOLOGICAL: - log << "reverse topological"; - break; - case OrderOfSCCs::DECREASING: - log << "decreasing"; - break; - case OrderOfSCCs::INCREASING: - log << "increasing"; - break; - } - log << endl; - - log << "Merge strategy for merging within sccs: " << endl; + log << "Merge strategy for merging individual SCCs/partitions: " + << endl; merge_selector->dump_options(log); } } @@ -157,31 +109,18 @@ class MergeStrategyFactorySCCsFeature "components (SCCs) of the causal graph, " "obtaining a partitioning of the task's variables. Every such " "partition is then merged individually, using the score-based merge " - "strategy specified via the merge_selector option. If " - "allow_working_on_all_clusters=true, then all pairs of factors in " - "each partition form the set of candidates scored by the score-based " - "merge strategy. Otherwise, SCC partitions are worked on 'one after " - "the other' in the order specified via 'order_of_sccs', and hence " - "only the pairs of factors of the 'current partition' form the set " - "of candidates. In both cases, once all partitions have been merged, " - "the resulting product factors are merged according to the score-" - "based merge strategy."); + "strategy specified via the merge_selector option. Once all partitions " + "have been merged, the resulting product factors are merged according " + "to the score-based merge strategy."); document_note( - "Note regarding allow_working_on_all_clusters", - "The option allow_working_on_all_clusters was not introduced in the " - "original paper, hence to obtain exactly the configurations of that paper, " - "set the option to false."); - add_option( - "order_of_sccs", - "how the SCCs should be ordered (only relevant if allow_working_on_all_clusters=false)", - "topological"); + "Note regarding how partitions are considered", + "Originally, SCC partitions are worked on 'one after the other' " + "in an order that could be specified and which defaulted to topological " + "order. In issue1171, this was changed to 'allowing working on any " + "partition', i.e., all pairs of factors in each partition form the set " + "of candidates scored by the score-based merge strategy."); add_option>( "merge_selector", "the fallback merge strategy to use"); - add_option( - "allow_working_on_all_clusters", - "if true, consider as merge candidates the pairs of factors of all SCCs. If " - "false, fully finish dealing with one cluster at a time.", - "true"); add_merge_strategy_options_to_feature(*this); } @@ -189,24 +128,11 @@ class MergeStrategyFactorySCCsFeature const plugins::Options &opts) const override { return components::make_auto_task_independent_component< MergeStrategyFactorySCCs, MergeStrategyFactory>( - opts.get("order_of_sccs"), opts.get>( "merge_selector"), - opts.get("allow_working_on_all_clusters"), get_merge_strategy_arguments_from_options(opts)); } }; static plugins::FeaturePlugin _plugin; - -static plugins::TypedEnumPlugin _enum_plugin( - {{"topological", - "according to the topological ordering of the directed graph " - "where each obtained SCC is a 'supervertex'"}, - {"reverse_topological", - "according to the reverse topological ordering of the directed " - "graph where each obtained SCC is a 'supervertex'"}, - {"decreasing", "biggest SCCs first, using 'topological' as tie-breaker"}, - {"increasing", - "smallest SCCs first, using 'topological' as tie-breaker"}}); } diff --git a/src/search/merge_and_shrink/merge_strategy_factory_sccs.h b/src/search/merge_and_shrink/merge_strategy_factory_sccs.h index 2d0c0c016a..aae40e3e03 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.h +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.h @@ -6,26 +6,16 @@ namespace merge_and_shrink { class MergeSelector; -enum class OrderOfSCCs { - TOPOLOGICAL, - REVERSE_TOPOLOGICAL, - DECREASING, - INCREASING -}; - class MergeStrategyFactorySCCs : public MergeStrategyFactory { - OrderOfSCCs order_of_sccs; std::shared_ptr merge_selector; - bool allow_working_on_all_clusters; protected: virtual std::string name() const override; virtual void dump_strategy_specific_options() const override; public: MergeStrategyFactorySCCs( const std::shared_ptr &task, - const OrderOfSCCs &order_of_sccs, const std::shared_ptr &merge_selector, - bool allow_working_on_all_clusters, utils::Verbosity verbosity); + utils::Verbosity verbosity); virtual std::unique_ptr compute_merge_strategy( const TaskProxy &task_proxy, const FactoredTransitionSystem &fts) override; diff --git a/src/search/merge_and_shrink/merge_strategy_sccs.cc b/src/search/merge_and_shrink/merge_strategy_sccs.cc index a1d5c6f9db..64b9ed167b 100644 --- a/src/search/merge_and_shrink/merge_strategy_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_sccs.cc @@ -14,12 +14,10 @@ namespace merge_and_shrink { MergeStrategySCCs::MergeStrategySCCs( const FactoredTransitionSystem &fts, const shared_ptr &merge_selector, - vector> &&unfinished_clusters, - bool allow_working_on_all_clusters) + vector> &&unfinished_clusters) : MergeStrategy(fts), merge_selector(merge_selector), - unfinished_clusters(move(unfinished_clusters)), - allow_working_on_all_clusters(allow_working_on_all_clusters) { + unfinished_clusters(move(unfinished_clusters)) { } MergeStrategySCCs::~MergeStrategySCCs() { @@ -43,36 +41,25 @@ pair MergeStrategySCCs::get_next() { } else { // There are clusters we still have to deal with. vector> merge_candidates; - vector factor_to_cluster; - if (allow_working_on_all_clusters) { - // Compute merge candidate pairs for each cluster. - factor_to_cluster.resize(fts.get_size(), -1); - for (size_t cluster_index = 0; - cluster_index < unfinished_clusters.size(); ++cluster_index) { - const vector &cluster = unfinished_clusters[cluster_index]; - for (int factor : cluster) { - factor_to_cluster[factor] = cluster_index; - } - compute_merge_candidates(cluster, merge_candidates); + + // Compute merge candidate pairs for each cluster. + vector factor_to_cluster(fts.get_size(), -1); + for (size_t cluster_index = 0; + cluster_index < unfinished_clusters.size(); ++cluster_index) { + const vector &cluster = unfinished_clusters[cluster_index]; + for (int factor : cluster) { + factor_to_cluster[factor] = cluster_index; } - } else { - // Deal with first cluster. - vector &cluster = unfinished_clusters.front(); compute_merge_candidates(cluster, merge_candidates); } + // Select the next merge from the allowed merge candidates. pair next_pair = merge_selector->select_merge_from_candidates( fts, move(merge_candidates)); // Get the cluster from which we selected the next merge. - int affected_cluster_index; - if (allow_working_on_all_clusters) { - affected_cluster_index = factor_to_cluster[next_pair.first]; - assert( - affected_cluster_index == factor_to_cluster[next_pair.second]); - } else { - affected_cluster_index = 0; - } + int affected_cluster_index = factor_to_cluster[next_pair.first]; + assert(affected_cluster_index == factor_to_cluster[next_pair.second]); // Remove the two merged indices from that cluster. vector &affected_cluster = diff --git a/src/search/merge_and_shrink/merge_strategy_sccs.h b/src/search/merge_and_shrink/merge_strategy_sccs.h index f79f28710a..5ce40f228a 100644 --- a/src/search/merge_and_shrink/merge_strategy_sccs.h +++ b/src/search/merge_and_shrink/merge_strategy_sccs.h @@ -11,13 +11,11 @@ class MergeSelector; class MergeStrategySCCs : public MergeStrategy { std::shared_ptr merge_selector; std::vector> unfinished_clusters; - bool allow_working_on_all_clusters; public: MergeStrategySCCs( const FactoredTransitionSystem &fts, const std::shared_ptr &merge_selector, - std::vector> &&unfinished_clusters, - bool allow_working_on_all_clusters); + std::vector> &&unfinished_clusters); virtual ~MergeStrategySCCs() override; virtual std::pair get_next() override; };