Issue1171 #244

silvansievers · 2025-02-02T09:50:54Z

No description provided.

roeger · 2025-02-06T13:25:45Z

Hi @silvansievers! The integration of PR #243 lead to conflicts for this PR. Can you have a look into this?

silvansievers · 2025-02-09T19:43:38Z

Hi @silvansievers! The integration of PR #243 lead to conflicts for this PR. Can you have a look into this?

Done. This is now ready for experiments and reviews.

maltehelmert · 2025-07-23T08:48:23Z

src/search/merge_and_shrink/merge_strategy_sccs.cc

+        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);


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(); ).

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.

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; });```

maltehelmert · 2025-07-23T08:48:42Z

src/search/merge_and_shrink/merge_selector.cc

+    const FactoredTransitionSystem &fts) const {
+    vector<int> indices;
+    indices.reserve(fts.get_num_active_entries());
+    for (int index: fts) {


space before ":" ?

maltehelmert · 2025-07-23T08:53:37Z

src/search/merge_and_shrink/merge_selector_score_based_filtering.cc

-    vector<pair<int, int>> merge_candidates =
-        compute_merge_candidates(fts, indices_subset);
-
+    vector<pair<int, int>> &&merge_candidates) const {


I don't understand why merge_candidates is an rvalue reference here, and the assignment to the rvalue reference confuses me. Can this be a regular reference instead, or is the intended semantics different?

An rvalue reference doesn't make sense indeed. I think I ended up using it because a default value cannot be bound to a non-cost reference. Also, the passed object doesn't need to be modified, i.e., from the caller's perspective, the right interface would indeed be a const reference. To summarize: I'd like to have an optional parameter merge_candidates that can be assigned to within the method in case it is empty (which is the default when the parameter is not specified by the caller).

Do you have any idea of how to achieve what I wrote? Could std::optional help?

what about having two separate functions?
One with the parameter and one without. Anyways, the code in compute_merge_candidates is entirely divided in two parts (by an if), so to me it would make sense to split it into two separate functions depending on whether there is already a set of candidates or not, and never have a check of whether the set of candidates is empty.

Also I think it makes more sense semantically, if I call the function
vector<pair<int, int>> MergeSelector::compute_merge_candidates(
const FactoredTransitionSystem &fts,
const vector &indices_subset) const {

with an empty indices_subset, the result should be an empty list because I didn't have a candidate in my set anyway :) The current semantics of (if my list has an element then you are not allowed to generate new candidates but if my list is empty then feel free to add as many candidates as you wish) is slightly more complex.

I like the suggestion and opened an issue for this. Here is another pull request for that change: #275

maltehelmert · 2025-07-23T08:55:06Z

src/search/merge_and_shrink/merge_strategy_factory_sccs.cc

+        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.",


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

…y 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.

silvansievers force-pushed the issue1171 branch from 0589c07 to befed26 Compare February 9, 2025 11:50

silvansievers force-pushed the issue1171 branch 2 times, most recently from c229381 to ec918bf Compare February 15, 2025 06:20

silvansievers force-pushed the issue1171 branch from ec918bf to 6e50368 Compare February 28, 2025 20:42

maltehelmert reviewed Jul 23, 2025

View reviewed changes

silvansievers force-pushed the issue1171 branch from 6e50368 to d401106 Compare October 1, 2025 20:52

style

201ee95

Issue1171 #244

Are you sure you want to change the base?

Issue1171 #244

Uh oh!

Conversation

silvansievers commented Feb 2, 2025

Uh oh!

roeger commented Feb 6, 2025

Uh oh!

silvansievers commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvansievers Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

silvansievers commented Feb 9, 2025 •

edited

Loading

silvansievers Aug 11, 2025 •

edited

Loading