From d40110633cba111f7065899d4e797be8cf75e4d0 Mon Sep 17 00:00:00 2001 From: Silvan Sievers Date: Sun, 2 Feb 2025 10:33:49 +0100 Subject: [PATCH 1/2] [issue1171] M&S SCC merge strategy: new option to allow working on any SCC Previously, the merge selector used for selecting pairs of M&S factors only considered one SCC partition after the other. With the new option set to true, it considers the pairs of factors from any cluster of factors that has not been fully merged yet. --- .../merge_strategy_factory_sccs.cc | 48 ++++++-- .../merge_strategy_factory_sccs.h | 2 + .../merge_and_shrink/merge_strategy_sccs.cc | 111 ++++++++++-------- .../merge_and_shrink/merge_strategy_sccs.h | 8 +- 4 files changed, 104 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 f8ccc76734..9276c7573d 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,13 @@ static bool compare_sccs_decreasing( MergeStrategyFactorySCCs::MergeStrategyFactorySCCs( 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(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 +100,10 @@ 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 { @@ -154,16 +160,34 @@ 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"); + "merge_selector", + "the score-based merge strategy used to merge factors within and across SCCs"); + 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); } @@ -172,7 +196,9 @@ class MergeStrategyFactorySCCsFeature return plugins::make_shared_from_arg_tuples( opts.get("order_of_sccs"), opts.get>("merge_selector"), - get_merge_strategy_arguments_from_options(opts)); + 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 083fd94e15..4135638c8f 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; @@ -23,6 +24,7 @@ class MergeStrategyFactorySCCs : public MergeStrategyFactory { MergeStrategyFactorySCCs( const OrderOfSCCs &order_of_sccs, const std::shared_ptr &merge_selector, + bool allow_working_on_all_clusters, utils::Verbosity verbosity); virtual std::unique_ptr compute_merge_strategy( const TaskProxy &task_proxy, diff --git a/src/search/merge_and_shrink/merge_strategy_sccs.cc b/src/search/merge_and_shrink/merge_strategy_sccs.cc index 1a8be31fa3..cd10b38039 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,84 @@ 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 201ee956f78f0668637cc4171d90f2deff4e0bee Mon Sep 17 00:00:00 2001 From: Silvan Sievers Date: Wed, 1 Oct 2025 22:54:50 +0200 Subject: [PATCH 2/2] style --- .../merge_strategy_factory_sccs.cc | 10 +++------- .../merge_strategy_factory_sccs.h | 3 +-- .../merge_and_shrink/merge_strategy_sccs.cc | 19 +++++++++++-------- 3 files changed, 15 insertions(+), 17 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 9276c7573d..cbb9dc063c 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.cc @@ -34,8 +34,7 @@ static bool compare_sccs_decreasing( MergeStrategyFactorySCCs::MergeStrategyFactorySCCs( const OrderOfSCCs &order_of_sccs, const shared_ptr &merge_selector, - bool allow_working_on_all_clusters, - utils::Verbosity verbosity) + bool allow_working_on_all_clusters, utils::Verbosity verbosity) : MergeStrategyFactory(verbosity), order_of_sccs(order_of_sccs), merge_selector(merge_selector), @@ -100,9 +99,7 @@ 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); } @@ -197,8 +194,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) - ); + 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 4135638c8f..85e4aaf4f2 100644 --- a/src/search/merge_and_shrink/merge_strategy_factory_sccs.h +++ b/src/search/merge_and_shrink/merge_strategy_factory_sccs.h @@ -24,8 +24,7 @@ class MergeStrategyFactorySCCs : public MergeStrategyFactory { MergeStrategyFactorySCCs( const OrderOfSCCs &order_of_sccs, const std::shared_ptr &merge_selector, - bool allow_working_on_all_clusters, - 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 cd10b38039..a1d5c6f9db 100644 --- a/src/search/merge_and_shrink/merge_strategy_sccs.cc +++ b/src/search/merge_and_shrink/merge_strategy_sccs.cc @@ -26,8 +26,7 @@ MergeStrategySCCs::~MergeStrategySCCs() { } static void compute_merge_candidates( - const vector &indices, - vector> &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) { @@ -48,7 +47,8 @@ pair MergeStrategySCCs::get_next() { 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) { + 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; @@ -61,32 +61,35 @@ pair MergeStrategySCCs::get_next() { compute_merge_candidates(cluster, merge_candidates); } // Select the next merge from the allowed merge candidates. - pair next_pair = merge_selector->select_merge_from_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]); + assert( + affected_cluster_index == factor_to_cluster[next_pair.second]); } else { affected_cluster_index = 0; } // Remove the two merged indices from that cluster. - vector &affected_cluster = unfinished_clusters[affected_cluster_index]; + 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 { + } else { ++it; } } if (affected_cluster.empty()) { // If the cluster got empty, remove it. - unfinished_clusters.erase(unfinished_clusters.begin() + affected_cluster_index); + unfinished_clusters.erase( + unfinished_clusters.begin() + affected_cluster_index); } else { // Otherwise, add the index of the to-be-created product factor. affected_cluster.push_back(fts.get_size());