feat: symmetry breaking (to be evaluated)#10
Conversation
flowpaths/abstractpathmodeldag.py
Outdated
| # safe_lists may contain many more entries than k; cap it so the range below is never empty | ||
| # when there are genuinely free paths to order. | ||
| if hasattr(self, "paths_to_fix") and self.paths_to_fix is not None: | ||
| n_fixed = len(self.paths_to_fix) | ||
| else: | ||
| # quite likely, this is higher than k, so then this method has no effect. | ||
| n_fixed = len(self.safe_lists) | ||
|
|
||
| free_pair_indices = list(range(n_fixed + 1, self.k)) |
There was a problem hiding this comment.
One concern I have is that, in its current form, this will be pretty useless because the paths_to_fix variable is not set before this method runs. I don't have a super good overview of the different execution paths from subclasses. Is paths_to_fix even the correct variable to look at here? Or do we need to make this a method that subclasses have to call separately, after adding all the other variables?
I also realize that the in-code comment is a bit outdated, so ignore it -- the check for empty index list / no paths to order comes right after this now.
Basically, @alexandrutomescu can you review these few lines please :D
There was a problem hiding this comment.
Yes, paths_to_fix is the variable to look at. Now issue addressed and merged into main. This uncovered a previous bug where the safety optimizations were now applied at all in DAGs, probably due to an earlier incorrect refactoring.
2 participants
This adds symmetry breaking among all paths that are not fixed already, in the hope to significantly reduce the number of branches that need to be explored when solving.
Marked as draft as this still needs evaluation and proof of correctness. So far, it is only anecdotally tested. For small k, it slows down computation (because the number of solver variables is bigger), however for mid-single-digit-k-s is starts to show value, but only on some test instances. So far, only tested with k-Min-Path-Error models. The solver size is also significantly increased by all the new variables. I think we can get rid at least of the "diff" variables by substituting.