Max Constraint Fix implemented by Duligur #96
Conversation
|
Same context as the other pull request... MaxConstraint.cpp Line 107: if the LB is for f and not for one of the elements, what happens? And again, unit tests: this pull request is intended to fix some bugs, right? Please add unit tests describing those bugs, and see that they now pass. |
|
I am actually not sure what error this was supposed to fix; before Duligur left the project he mentioned to us that the existing Max Constraint would sometimes give him an error, and this was his re-implementation that worked. I just copied the files from his fork in this commit. Hopefully @ibeling can help explain why this was needed. |
|
Okay, lets wait to see what Duligur says |
|
Hey, I just met with Shantanu, Parth, and Clark, just writing down what I mentioned. I don't 100% understand the old code--I wrote this mostly from scratch. My guess, however, is that at least one bug has something to do with lines 113-114 of MaxConstraint.cpp. If you do not include these lines, it is possible for a variable to eliminate itself if its bounds updates are received out of order; and I don't think the old code had this. I have also removed the "internal bound propagation" of the old code, and the calling of eliminateVariable outside of preprocessing. Guy, re: line 107, that is right, a lower bound for f should be able to eliminate those elements that have upper bounds less than the lower bound; I forgot to do these eliminations. I believe that affects performance only, but it would be very important to add this in. |
* Copied some files * Simple check for obsolete if phase is fixed * Better obsolete()
Just copied correct version of max constraint implemented by @ibeling in his fork.