-
Notifications
You must be signed in to change notification settings - Fork 257
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
add support for user-provided decompositions #842
base: master
Are you sure you want to change the base?
Conversation
860b888
to
8beb978
Compare
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.
Puhhhhhhh. I misunderstood this MR a bit at the start. Not the smartest bear in the woods.
I'm not an expert with decompositions, and am not sure what general users want from this functionality. I will need to look into how SCIP handles them before I can check if everything correct.
In the meantime: More tests and comments are needed. I am against adding such functionality without making it extremely clear to users what it does. This is just a draft though, so I guess speed isn't a concern right now.
decomp.scip_decomp = scip_decomp | ||
return decomp | ||
|
||
def isOriginal(self): |
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.
We're trying to make sure new functionality isn't added without having tests. So for all available functions you're adding please put them into a simple test (one test can cover multiple functions)
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.
Will add more tests & docs when I get a moment, sorry. This one got put on the backburner due to other stuff at work.
For our use case, we have a modeling layer on top of PySCIPOpt that basically uses dataframe-like syntax to create groups of constraints using a table that characterizes our variables. So the goal is to break .groupby
statements into subproblems where each of the groups can be solved ~in parallel. We have some combinatorial constraints as well, so decompositions seem like a natural place to reduce the number of combinations to check.
def isOriginal(self): | ||
return SCIPdecompIsOriginal(self.scip_decomp) | ||
|
||
def getAreaScore(self): |
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.
As this isn't standard knowledge (or at least I don't know it), if you can add a one line comment for what the function does then please do. This really helps out users and means they don't need to go through SCIP itself for answers.
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.
This extends to most functions below
this commit will add support for user-provided decompositions as dicts of
{var: label}
,{cons:label}
pairs.still needs testing & examples, but opening a draft so maintainers can follow along if you have time.