Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pickle transformed GDP models #2576

Merged
merged 10 commits into from
Nov 2, 2022
Merged

Pickle transformed GDP models #2576

merged 10 commits into from
Nov 2, 2022

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Oct 25, 2022

Fixes #2417

Summary/Motivation:

The bigm and hull transformations create Blocks for each Disjunct they transform, each with a weakref back to the original component. This PR makes those Blocks into a new component that defines an __autoslot_mappers__ dict so that the transformed model can be pickled.

Changes proposed in this PR:

  • Defines _TransformedDisjunct class, which is just a Block with a weakref on it
  • Adds __autoslot_mappers__ dicts to Disjuncts and Disjunctions
  • Tests pickling GDP models transformed with bigm and hull

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@blnicho blnicho requested a review from jsiirola October 25, 2022 19:04
@codecov
Copy link

codecov bot commented Oct 28, 2022 •
edited
Loading

Codecov Report

Base: 86.76% // Head: 86.76% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (4bcf833) compared to base (65b9a58).
Patch coverage: 96.96% of modified lines in pull request are covered.

❗ Current head 4bcf833 differs from pull request most recent head da08461. Consider uploading reports for the commit da08461 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2576      +/-   ##
==========================================
- Coverage   86.76%   86.76%   -0.01%     
==========================================
  Files         726      726              
  Lines       81228    81211      -17     
==========================================
- Hits        70478    70459      -19     
- Misses      10750    10752       +2
Flag Coverage Δ
linux 84.14% <96.96%> (+<0.01%) ⬆️
osx 74.48% <96.96%> (+<0.01%) ⬆️
other 84.32% <96.96%> (+<0.01%) ⬆️
win 80.93% <96.96%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/gdp/transformed_disjunct.py 94.11% <94.11%> (ø)
pyomo/gdp/disjunct.py 85.82% <100.00%> (+0.10%) ⬆️
pyomo/gdp/plugins/bigm.py 95.01% <100.00%> (+0.01%) ⬆️
pyomo/gdp/plugins/cuttingplane.py 94.94% <100.00%> (ø)
pyomo/gdp/plugins/hull.py 93.45% <100.00%> (+0.01%) ⬆️
pyomo/gdp/util.py 92.25% <100.00%> (ø)
pyomo/core/expr/visitor.py 93.21% <0.00%> (-0.16%) ⬇️
pyomo/solvers/plugins/solvers/mosek_direct.py 68.59% <0.00%> (-0.05%) ⬇️
examples/kernel/conic.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jsiirola
jsiirola reviewed Oct 31, 2022
View reviewed changes
Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple quick questions. Also, can we add a test somewhere to help with patch coverage?

pyomo/gdp/transformed_disjunct.py Outdated Show resolved Hide resolved
pyomo/gdp/transformed_disjunct.py Outdated Show resolved Hide resolved
@emma58
Copy link
Contributor Author

emma58 commented Nov 1, 2022

@jsiirola, so, I think I was being a bit silly: Probably the patch coverage is low because we never actually make ScalarTransformedDisjuncts... Should we just not define it? Or I can make one in a test, but I don't really know what it's for.

@jsiirola
Copy link
Member

jsiirola commented Nov 1, 2022

There really isn't a use case for the scalar, is there? It would have to be a model with a single 1-term disjunction?

If that's the case, then I think the right thing is to remove the scalar class entirely. If there is only indexed, then you can also delete the __new__ stuff, too!

jsiirola
jsiirola approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple (minor) questions, but this looks good!

pyomo/gdp/transformed_disjunct.py Outdated Show resolved Hide resolved
pyomo/gdp/transformed_disjunct.py Outdated Show resolved Hide resolved
@emma58 emma58 merged commit f6606f9 into Pyomo:main Nov 2, 2022
@emma58 emma58 deleted the fix-transformed-gdp-pickle branch November 2, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsiirola jsiirola jsiirola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transformed GDP models don't pickle with non-dill pickles
3 participants
@emma58 @jsiirola @pyomo-autotest