Skip to content

Issue1171 #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
silvansievers wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Issue1171 #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d401106
[issue1171] M&S SCC merge strategy: new option to allow working on an…
Feb 2, 2025
201ee95
style
Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions src/search/merge_and_shrink/merge_strategy_factory_sccs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ static bool compare_sccs_decreasing(

MergeStrategyFactorySCCs::MergeStrategyFactorySCCs(
const OrderOfSCCs &order_of_sccs,
const shared_ptr<MergeSelector> &merge_selector, utils::Verbosity verbosity)
const shared_ptr<MergeSelector> &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<MergeStrategy> MergeStrategyFactorySCCs::compute_merge_strategy(
Expand Down Expand Up @@ -97,7 +99,8 @@ unique_ptr<MergeStrategy> MergeStrategyFactorySCCs::compute_merge_strategy(
}

return make_unique<MergeStrategySCCs>(
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 {
Expand Down Expand Up @@ -154,16 +157,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<OrderOfSCCs>(
"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<shared_ptr<MergeSelector>>(
"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<bool>(
"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.",
Copy link
Contributor

@maltehelmert maltehelmert Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps simpler: "If false, deal with one cluster at a time."?

"true");
add_merge_strategy_options_to_feature(*this);
}

Expand All @@ -172,6 +193,7 @@ class MergeStrategyFactorySCCsFeature
return plugins::make_shared_from_arg_tuples<MergeStrategyFactorySCCs>(
opts.get<OrderOfSCCs>("order_of_sccs"),
opts.get<shared_ptr<MergeSelector>>("merge_selector"),
opts.get<bool>("allow_working_on_all_clusters"),
get_merge_strategy_arguments_from_options(opts));
}
};
Expand Down
3 changes: 2 additions & 1 deletion src/search/merge_and_shrink/merge_strategy_factory_sccs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ enum class OrderOfSCCs {
class MergeStrategyFactorySCCs : public MergeStrategyFactory {
OrderOfSCCs order_of_sccs;
std::shared_ptr<MergeSelector> 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 OrderOfSCCs &order_of_sccs,
const std::shared_ptr<MergeSelector> &merge_selector,
utils::Verbosity verbosity);
bool allow_working_on_all_clusters, utils::Verbosity verbosity);
virtual std::unique_ptr<MergeStrategy> compute_merge_strategy(
const TaskProxy &task_proxy,
const FactoredTransitionSystem &fts) override;
Expand Down
114 changes: 64 additions & 50 deletions src/search/merge_and_shrink/merge_strategy_sccs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <algorithm>
Expand All @@ -16,71 +14,87 @@ namespace merge_and_shrink {
MergeStrategySCCs::MergeStrategySCCs(
const FactoredTransitionSystem &fts,
const shared_ptr<MergeSelector> &merge_selector,
vector<vector<int>> &&non_singleton_cg_sccs)
vector<vector<int>> &&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<int> &indices, vector<pair<int, int>> &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<int, int> 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<pair<int, int>> merge_candidates;
vector<int> 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<int> &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<int> &current_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<int> &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<int, int> next_pair = merge_selector->select_merge_from_candidates(
fts, move(merge_candidates));

// Compute all merge candidates for the current set of indices.
vector<pair<int, int>> 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<int, int> next_pair = merge_selector->select_merge_from_candidates(
fts, move(merge_candidates));
// Remove the two merged indices from that cluster.
vector<int> &affected_cluster =
unfinished_clusters[affected_cluster_index];
for (vector<int>::iterator it = affected_cluster.begin();
it != affected_cluster.end();) {
if (*it == next_pair.first || *it == next_pair.second) {
it = affected_cluster.erase(it);
Copy link
Contributor

@maltehelmert maltehelmert Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read https://en.cppreference.com/w/cpp/container/vector/erase correctly, this invalidates the iterator it. I doubt a typical implementation would care, but this is probably still invalid C++. I suggest you access the vector with an index instead, i.e., for (size_t i = 0; i < affected_cluster.size(); ).

Copy link
Contributor Author

@silvansievers silvansievers Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly do you think is the problem? The parameter it gets invalidated, yes, but the return value is a new valid iterator. The example in the link uses the exact same if-else-pattern as the code here.

Copy link
Contributor

@maltehelmert maltehelmert Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I missed the assignment to it. So this works. Looking at this a bit more closely now, I am a bit queasy because this is a code smell -- repeatedly calling erase on a vector moves the rest of the vector on every erasure, and in generally this makes such filtering O(n^2) where it is O(n) when using STL algorithms like erase_if, which is what things like the C++ core guidelines would recommend in a case like this. Of course here it's still O(n) because we only remove two things, but erase_if (with a lambda) would still be the more idiomatic C++ approach.

Now that we require c++20, there is a nice special version of erase_if that just takes a vector and a predicate as an argument and does all the resizing etc.

Something like this could replace the for loop and be more efficient as well:

erase_if(affected_cluster, [&](int elem) {
    return elem == next_pair.first || elem == next_pair.second; });```

} else {
++it;
}
}

// Remove the two merged indices from the current index set.
for (vector<int>::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;
}
}
8 changes: 4 additions & 4 deletions src/search/merge_and_shrink/merge_strategy_sccs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@ namespace merge_and_shrink {
class MergeSelector;
class MergeStrategySCCs : public MergeStrategy {
std::shared_ptr<MergeSelector> merge_selector;
std::vector<std::vector<int>> non_singleton_cg_sccs;

std::vector<int> current_ts_indices;
std::vector<std::vector<int>> unfinished_clusters;
bool allow_working_on_all_clusters;
public:
MergeStrategySCCs(
const FactoredTransitionSystem &fts,
const std::shared_ptr<MergeSelector> &merge_selector,
std::vector<std::vector<int>> &&non_singleton_cg_sccs);
std::vector<std::vector<int>> &&unfinished_clusters,
bool allow_working_on_all_clusters);
virtual ~MergeStrategySCCs() override;
virtual std::pair<int, int> get_next() override;
};
Expand Down
Loading