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.

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

contrib.piecewise: Adding nonlinear-to-piecewise-linear transformation #3333

Merged
merged 58 commits into from
Aug 20, 2024

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Aug 5, 2024

Fixes # .

Summary/Motivation:

This PR adds a transformation that takes a MINLP model and automatically generated a piecewise-linear approximation of the model that can then be transformed to MILP using the other transformations in contrib.piecewise.

Changes proposed in this PR:

  • Adds contrib.piecewise.nonlinear_to_pwl transformation
  • Adds a lot of tests
  • Sneaks in a fix to correctly handle LogicalConstraints in the transformations from piecewise linear to GDP/MIP

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.

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 97.59760% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.60%. Comparing base (29f8ede) to head (b3429e1).
Report is 325 commits behind head on main.

Files Patch % Lines
...mo/contrib/piecewise/transform/nonlinear_to_pwl.py 97.83% 7 Missing ⚠️
...omo/contrib/piecewise/piecewise_linear_function.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3333      +/-   ##
==========================================
+ Coverage   88.55%   88.60%   +0.04%     
==========================================
  Files         877      878       +1     
  Lines       99262    99591     +329     
==========================================
+ Hits        87906    88238     +332     
+ Misses      11356    11353       -3     
Flag Coverage Δ
linux 86.13% <97.59%> (+0.03%) ⬆️
osx 75.83% <97.59%> (+0.07%) ⬆️
other 86.64% <97.59%> (+0.04%) ⬆️
win 83.98% <97.59%> (+0.05%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emma58
Copy link
Contributor Author

emma58 commented Aug 13, 2024

Oops, I think this is failing coverage because we don't have lineartree installed anywhere for testing...

@emma58
Copy link
Contributor Author

emma58 commented Aug 13, 2024

If this passes tests, it is ready for review!

@emma58
Copy link
Contributor Author

emma58 commented Aug 14, 2024

@mrmundt @jsiirola Are the pypy failures my fault? They seem consistent.

@jsiirola
Copy link
Member

@mrmundt @jsiirola Are the pypy failures my fault? They seem consistent.

Yes - I think so. Because you added linear-tree, that triggered pip to install scipy, which is known to have issues under pypy (which is why we exclude it).

I think the resolution is to add linear-tree to PYPY_EXCLUDE: scipy numdifftools seaborn statsmodels in .github/workflows/test_pr_and_main.yml and .github/workflows/test_branches.yml. Note that you need to update .github/workflows/test_branches.yml to add linear-tree to PYPI_ONLY , too...

@emma58
Copy link
Contributor Author

emma58 commented Aug 15, 2024

Oooof, now I broke everything?

@blnicho
Copy link
Member

blnicho commented Aug 15, 2024

@emma58 GitHub was having issues yesterday so I just kicked off the GHA tests again to see if that fixes things.

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.

Overall, this looks pretty good. One thing that should be changed, though (defer the construction of the visitor, or move it to an instance attribute)

Comment on lines 70 to 72
_quadratic_repn_visitor = QuadraticRepnVisitor(
subexpression_cache={}, var_map={}, var_order={}, sorter=None
)
Copy link
Member

Choose a reason for hiding this comment

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

This gets instantiated upon module import (which will happen as part of pyomo.environ. It would be better to put this in a getter:

def visitor():
    if visitor._instance is None:
        visitor._instance = QuadraticRepnVisitor(
            subexpression_cache={}, var_map={}, var_order={}, sorter=None
        )
    return vistor._instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it on the transformation instance.

@blnicho blnicho merged commit 3ea08c9 into Pyomo:main Aug 20, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants