-
Notifications
You must be signed in to change notification settings - Fork 269
Recipe for getting local constraints #1014
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new recipe for retrieving local constraints in a SCIP node, along with appropriate tests and documentation updates.
- Added functions getLocalConss and getNLocalConss in the pyscipopt/recipes module.
- Enhanced test coverage in tests/test_recipe_getLocalConss.py to verify constraint retrieval behavior.
- Updated CHANGELOG.md to document the new feature.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/test_recipe_getLocalConss.py | Added tests to verify the local constraints feature. |
src/pyscipopt/recipes/nonlinear.py | Removed an extra blank line for cleanup. |
src/pyscipopt/recipes/getLocalConss.py | Added new functions for retrieving local constraints. |
CHANGELOG.md | Documented the addition of the new recipe. |
Comments suppressed due to low confidence (1)
tests/test_recipe_getLocalConss.py:9
- [nitpick] The variable name 'vars' shadows the built-in Python function vars(), which can lead to confusion. Consider renaming it to something more descriptive like 'model_vars'.
vars = model.getVars()
Everything seems fine, will merge in a couple days, if everyone's alright with it. |
Better iterate through the constraint handlers and get the active constraints using https://scipopt.org/doc-9.2.2/html/group__PublicConshdlrMethods.php#gae573c332a19e69c45163fadfeab3948b and https://scipopt.org/doc-9.2.2/html/group__PublicConshdlrMethods.php#gad610bfb6d34ee998835d8b9bbbb9cf8c and exclude original ones manually. |
Why is it better? You mean that some constraints are discarded for whatever reason, but they would appear here? |
It should also be checked that the constraints are enabled, but I rather wonder about the performance impact of walking back to the root for each node. If local constraints are added for every node, this might be okay though. However, I am not sure about the usecase of this set of constraints. |
@DominikKamp mostly for debugging. But it seems useful to be able to get the constraints you added after starting to solve. Especially since users don't have an easy way to get the constraints that make up the local problem. I guess interfacing Regarding performance, it doesn't seem that bad, plus it's a recipe. |
But global constraints are also local constraints so the method name is actually misleading. |
What would you name it? I can also return all the local constraints (including original ones), but if users are interested in the non-original ones, they would need to dig through the array. One option would actually be to return a two-dimensional array instead. First entry with original constraints, second entry with non-original constraints. |
Since you prepend the added constraints, you could also just include the root constraints and additionally provide their number, or even better return an array with all the lengths. |
See how you like it now, @DominikKamp. It might be better if we just return EDIT: but then retrieving the constraints is very annoying, would need plenty of indices. I'm happy with this for now, since it's a recipe no need for perfection. |
Actually I meant just two lists, one for all enabled constraints added on the whole path in sequence, another one with the number of constraints for each node on the path. Note, that all constraints disabled in the current node need to be excluded manually. Furthermore, you might want to skip the nodes before the effective root. |
If we're going to discriminate the number of constraints by node, then I'd favor a dictionary with node numbers as keys, otherwise, it seems like a mess. The constraints are disabled, but still valid, no? Why would it matter if they're there or not? I'd rather have them, and then people check if they're active manually, otherwise it seems like they disappeared. The effective root is a term I'm not familiar with, what do you mean? In theory, we could just check between nodes A and its ancestor B, but I'm assuming that B is always the actual root. |
Fine with a dictionary for the numbers of constraints (though it might make it more difficult to access the number of global constraints). Disabled constraints are technically not part of the local description but should be valid, just thought that it is the purpose of this function to provide the current set of local constraints. By default, if all but one child of the current effective root is cut off, the single remaining child becomes the next effective root and all its bound and constraint set changes are globalized, but you actually need to go to the original root to get all global constraints. |
SCIP only allows one to get the constraints added to a given node. It is now possible to get all constraints valid in the focus node, excluding the original ones. Maybe we can also add a
getLocalConsNode
, or even an optional argument defaulting to the current node.