-
Notifications
You must be signed in to change notification settings - Fork 5
Addition of copy_model and copy_gdp_data to copy GDP models. #126
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
Conversation
|
@pulsipher this is ready for review. |
Refactor variable creation and add copy_model_and_gdp_data function.
pulsipher
left a comment
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.
Looking good, just some minor points to address
created more tests accordingly. addition of test to check that model instances do not affect each other.
pulsipher
left a comment
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.
Looks good, just one more thing. Please add some comments make the code more readable. For example, a comment for each for loop in copy_gdp_data and some comments for the dispatch functions. Also, the new public functions should each have a docstring.
pulsipher
left a comment
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.
Looks good to me.
This PR extends
JuMP.copy_extension_datain order to allowGDPModels to be an input toJuMP.copy_model().JuMP.copy_extension_datadoes not receive a reference map as one of it's argument so it's impossible to recover disjunct constraints. Therefore another function was made to be called (DP.copy_gdp_data()) in order to populate the new model'sGDPData. The workflow goes sequentiallty as in the example below.In addition there is also
copy_model_and_gdp_datato do bothAn alternative would be to extend JuMP.copy_model itself but at a glance I believe there might be consequences when we want to have DP coexist with other packages and have the ability to copy.