Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 the support of GreyBox in MindtPy #2988
Add the support of GreyBox in MindtPy #2988
Changes from 19 commits
bb03a18
1d508e7
ca54afc
ec05a3d
5048b42
c2877a3
ccff416
e461e93
13a7213
20ccbdc
4288e33
8d7f6c5
807e4f5
30771b3
231abc6
72b142b
ff401df
d82dcde
cb1c2a9
e80c6dc
8959666
e0c245b
5ffaeb1
260b2d6
c10e212
46cf8a5
ba135b6
8eacf0b
bd12664
ebe91a6
2b45756
96efbd4
73f2e16
a08fd36
ae84558
7bf0268
993290f
3cf0fc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do you really want to store this publicly on the solver object? If your users can find it and it doesn't start with underscore, they might start to rely on it. That's not necessarily bad--just something to think through.
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.
The use of this transformation is new. Is it motivated by our new tests? Is the transformation well-supported? @emma58
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.
After MindtPy rewrite, we repeatedly use the fixed-nlp subproblem. Therefore, we will call
'contrib.deactivate_trivial_constraints'
transformation and revert it after solving. I missed the feasibility subproblem and this might be a bug.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 makes me think that
load_solutions
shouldn't be up to the user then, since there are cases where you need to force it to be False.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.
Yes. This option should not be up to the users. Is there a way to define an option (not open to users) instead of config (open to users) in Pyomo?
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.
It's not really an option at all, just something you calculate based on some of the config options, right? You could probably store it privately on the algorithm class. Just make sure to restore state afterwards if you do keep it on the class.
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.
Is it a good choice to change
load_solutions
to the attribute of the Solver Object?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 sounds fine to me. You should probably make it private as well. The only risk of doing this is that you should be careful to restore the state after the solve.
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.
See my comment above, but I'm not sure of why this should be up to the user. I think it would be fine (and perhaps easier to support in the future) to keep this detail locally, or maybe privately on the solver object, and manage it yourself. Especially since it looks users don't actually have a choice if they want to use appsi solvers.
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.
Are you sure that this is always going to create a correct cut, even if greybox changes its behavior? The part that looks potentially scary to me is calling
enumerate
over getting thevalues()
from dictionaries coming from greybox. You will get deterministic ordering fromvalues()
as long as the dictionary is constructed in the same order every time, but if that changes, is this still right? Or does greybox make a promise about consistency with that order and the jacobian? (This is me wondering out of ignorance, but it may be worth adding a comment even if it is fine.)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.
If we don't call
enumerate
overvalues
, any suggestions about how to implement it? Directlyenumerate
overvariable
?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 don't really know--you would know better than me. It's completely possible that this is fine. My concern is that you will end up with the wrong coefficient (
jacobian_matrix[index][var_index]
) paired with the wrongvar - value(var)
term if thetarget_model_greyboox.inputs
dictionary is every constructed in a different order that doesn't correspond to the indices injacobian_matrix
. I don't know greybox well at all, so maybe it makes a promise that what I just said will never happen, in which case your code is totally fine. I'm just asking if you know if that's the case or not. It's always worth thinking carefully when you are relying on the order things come up when iterating over a dictionary...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.
Currently, I cannot come up with a good way to improve this since the
evaluate_jacobian_outputs
returns ascipy.sparse._coo.coo_matrix
.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.
Maybe just put in a comment that you are relying on the order in which items are added to the
target_model_greybox.inputs
dictionary, so that future you (or someone else) will have a hint if this ever doesn't work.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 become an issue?
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.
Gurobi
will fail even when we deactivate thegreybox
block. Therefore, we cannot useGurobi
as the mip solver.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.
Is this a bug in
gurobi_persistent
? Why does it behave differently?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 will submit an issue related to 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.
@emma58 Could you provide me a
reference
example?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 is the main idea. You'll want to figure out how to do your own accounting in terms of finding the Vars you need to reference and naming the references, etc.
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.
#3000 Issue opened
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.
@emma58 . I don't think
Reference
is an elegant way to resolve this issue. I will comment the code of addingGurobi
lazy constraints and only supportgreybox
in multi-tree implementation this time.Btw, since persistent solver will be totally replaced by
appsi_solver
, does appsi_solver support bothCPLEX
andGurobi
callbacks aspersistent solver
now? @michaelbynumThere 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.
appsi_gurobi does support callbacks. appsi_cplex will, but does not currently.