-
Notifications
You must be signed in to change notification settings - Fork 17
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
First version of symmip
, which adds tentative MIP support
#370
base: master
Are you sure you want to change the base?
Conversation
Highs would be wonderful to add as it is open source. |
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.
Alright! I mostly like it. I do have some comments and questions though.
- Can you explain what the problem is with Models and Objectives + MIP? As I see it it should be possible to hook at least Model into this, maybe as MIPModel (see also ODEModel). I agree (for now) to leave out Objectives.
- What does this mean for Indexed in base symfit?
- I'm missing tests and docs ;)
examples/mip/bilinear.py
Outdated
|
||
mip = MIP(- objective, constraints=constraints) | ||
# For full control, we still need to access the model directly. | ||
mip.backend.model.Params.NonConvex = 2 |
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.
Oh I don't like this at all...
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.
Nope, it's horrible. The good news is that it is possible however, which means that we don't have to wrap everything even to get started. But we still need to think about a syntax to deal with this. Personally I think that it might be best if the backend class GurobiBackend accepts any options, which will then be passed on to gurobi without looking into it.
The example above would then become
backend = GurobiBackend(NonConvex=2)
mip = MIP(- objective, constraints=constraints, backend=backend)
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.
That looks like an excellent option
mip.backend.model.Params.NonConvex = 2 | ||
mip_result = mip.execute() | ||
|
||
print(mip_result) |
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.
Missing empty line
# This example formulates and solves the following simple bilinear model: | ||
# maximize x | ||
# subject to x + y + z <= 10 | ||
# x * y <= 2 (bilinear inequality) | ||
# x * z + y * z = 1 (bilinear equality) | ||
# x, y, z non-negative (x integral in second version) |
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.
For my understanding, this is not a MIP, but just an almost-linear problem? In other words, there are no integer constraints. In other other words, you could solve this with the existing minimizers
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.
Second note, I don't see the non-negative boundaries in the code below
examples/mip/mip1.py
Outdated
f"y={fit_result[y]}, " | ||
f"z={fit_result[z]}" | ||
) | ||
print(fit_result) |
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.
Missing empty line
symfit/symmip/backend/gurobi.py
Outdated
from symfit.core.argument import Parameter | ||
|
||
|
||
VTYPES = {'binary': 'B', 'integer': 'I', 'real': 'C',} |
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.
Should this be a GurobiBackend class variable?
symfit/symmip/mip.py
Outdated
|
||
DummyModel = namedtuple('DummyModel', 'params') | ||
|
||
VTYPES = {'binary': 'B', 'integer': 'I', 'real': 'C',} # ToDo: Enum? |
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.
Seeing that this got duplicated here there has to be a better way
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.
agreed. I've now somewhat fixed this: in mip.py we only specify which types of variables there are as a list, whereas the specific backends can (should) dictate how these are to be translated to vtypes.
VTYPES = {'binary': 'B', 'integer': 'I', 'real': 'C',} # ToDo: Enum? | ||
|
||
|
||
class MIPBackend(Protocol): |
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.
Should the GurobiBackend be a subclass of this?
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.
For now I went with a protocol because it allows for static typing and duck-typing. An ABC was not worth it (yet) because there was no code reuse. But maybe I will make a common base class now that we have two solvers, but I'll keep the protocol nonetheless.
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.
I need to read up on what Protocol
does.
symfit/symmip/mip.py
Outdated
free_symbols = reduce(operator.or_, | ||
(c.free_symbols for c in self.constraints), | ||
self.objective.free_symbols if self.objective else set()) |
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.
... huh?
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.
Just taking the union of some sets of symbols! ;)
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.
Oh right. I can see how this works now. At minimal this needs a comment. Also consider using set.union
instead of operator.or_
.
Oh, and another question. I love the type annotations, but what does it mean for the minimum python version we can use/support? |
Thanks for the tip, I wasn't aware of that package! I agree that we should have an open solver (as a default). But I tried to include it and found the documentation lacking and moreover, couldn't get the examples to work for Highs. So I've now instead added a wrapper for SCIP and made that the default, and that seems to work beautifully! So thanks for pointing me in this direction. |
Great question. The main issue is that in order to translate the symbolic definition into a MIP model, I actually need to interact directly with the symbolic definition and use a custom code printer to translate it into the target MIP backend. I found that with our existing models it is actually quite hard to get back to the symbolic expressions, because they are more designed to be easily called with data and parameters. Moreover, with the existing Model classes there is no control over the Printer being used; it is always the default one that sympy offers. So my idea here was the following: within the
A bit similar to the previous answer, I think that starting from scratch in a new submodule in a context that is full of indexed symbols will be a good opportunity to learn some best practices for dealing wit these objects, that can then be back ported into symfit.
Yeah it definitely still needs some work ;).
I'm pretty sure that everything from >3.5 should be okay with this, so I don't expect any problems but let's keep our eyes open. |
Alright, thanks for the explanation. If I understand it Model + Minimizer determine the Printer? Or even just Minimizer? Following this line of thought, does that mean everything should remain as symbolic as possible, right up until the Minimizer touches it? This also has implications for Objectives --- symbolical objectives have been on my wish list for a while. I think it's a good idea to fix this in code symfit Models now that we (you) know better, similar for Indexed*. I think its fine to do some trial-and-error prototyping in a submodule like you do here, but I'd strongly prefer the final PR simply improving the symfit classes. This will also have the benefit of being able to use the tests to make sure you're not missing old use cases. I also think it'll result in less work since you'll be able to build on the existing framework. And where that doesn't fit, just break/adapt the old framework. Can you do MIPODEModels? If no, your design is not good enough yet :D |
…s which are not in model.
@@ -93,6 +95,9 @@ class Parameter(Argument): | |||
_argument_name = 'par' | |||
|
|||
def __new__(cls, name=None, value=1.0, min=None, max=None, fixed=False, **kwargs): | |||
if 'binary' in kwargs: | |||
kwargs['integer'] = kwargs['real'] = kwargs['nonnegative'] = True |
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.
kwargs['integer'] = kwargs['real'] = kwargs['nonnegative'] = True | |
kwargs['integer'] = True | |
kwargs['real'] = True | |
kwargs['nonnegative'] = True |
Co-authored-by: Peter C Kroon <pckroon@users.noreply.github.com>
First version of
symmip
, a symbolic wrapper around MIP solvers. Currently only support the Gurobi API, but the code is setup in such a way that we can easily add other solvers.Added as a seperate module, because merging it into the existing symfit infrastructure was less than obvious. In particular, the existing Models, objectives and minimizers are only very expensive boilerplate with no added value for MIPs, so for now I cut ties with this formalism. We can always still plug this MIP solver back into the Fit API if we have a good MIP API to begin with.