Skip to content

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Joao-Dionisio
Copy link
Member

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.

@Joao-Dionisio Joao-Dionisio requested a review from Copilot July 1, 2025 00:04
Copy link

@Copilot Copilot AI left a 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()

@Joao-Dionisio
Copy link
Member Author

Everything seems fine, will merge in a couple days, if everyone's alright with it.

@DominikKamp
Copy link
Contributor

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.

@Joao-Dionisio
Copy link
Member Author

Why is it better? You mean that some constraints are discarded for whatever reason, but they would appear here?
I'm just a bit fearful about returning transformed constraints.

@DominikKamp
Copy link
Contributor

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.

@Joao-Dionisio
Copy link
Member Author

Joao-Dionisio commented Jul 1, 2025

@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.
Initially, I thought about returning the local constraints plus the root constraints, but it would be less ergonomic for the user.

I guess interfacing SCIPwriteMIP makes sense, as well. Also, it's a bit ugly that both SCIPwriteLp and SCIPwriteLP exist.

Regarding performance, it doesn't seem that bad, plus it's a recipe.

@DominikKamp
Copy link
Contributor

But global constraints are also local constraints so the method name is actually misleading.

@Joao-Dionisio
Copy link
Member Author

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.

@DominikKamp
Copy link
Contributor

DominikKamp commented Jul 1, 2025

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.

@Joao-Dionisio
Copy link
Member Author

Joao-Dionisio commented Jul 4, 2025

See how you like it now, @DominikKamp. It might be better if we just return [[nglobal_conss, global_conss], [nadded_cons, added_conss]]. I don't know if this was what you were suggesting in the first place.

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.

@DominikKamp
Copy link
Contributor

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.

@Joao-Dionisio
Copy link
Member Author

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.

@DominikKamp
Copy link
Contributor

DominikKamp commented Jul 4, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants