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

Piecewise Linear transformations: Logarithmic and nested inner GDP representation #3036

Merged
merged 40 commits into from
May 8, 2024

Conversation

sadavis1
Copy link
Contributor

@sadavis1 sadavis1 commented Nov 14, 2023

Fixes # .

Summary/Motivation:

We want to add piecewise-linear to GDP and to MIP transformations from Vielma 2010 so they can be used in Bashar's PWL representations paper.

Changes proposed in this PR:

  • Add disaggregated_logarithmic transformation for PW linear, direct to mip as in Vielma
  • Add "nested_inner_repn" transformation, which should be a path to GDP-ification of Vielma's log transformations by identifying some variables, once some changes are made to gdp transformations to support it. Also, this transformation is somewhat interesting in its own right, because it is often faster than the inner representation despite not actually having the logarithmic variable count.
  • Rename class PiecewiseLinearToGDP to PiecewiseLinearTransformationBase, since it isn't GDP-specific and I'm using it for a 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
Contributor

@bernalde bernalde left a comment

Choose a reason for hiding this comment

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

Really cool PR!
Just saw a couple of typos! The interesting part would be the benchmarking once this is merged

@sadavis1
Copy link
Contributor Author

Fixed typos and some other minor issues.

Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

This is looking quite good!

@sadavis1
Copy link
Contributor Author

This should be ready now. The linux/3.12 test failure seems to be spurious.

@sadavis1 sadavis1 changed the title [WIP] Piecewise Linear transformations: Logarithmic and nested inner GDP representation Piecewise Linear transformations: Logarithmic and nested inner GDP representation Feb 15, 2024
@emma58 emma58 self-requested a review February 15, 2024 22:20
Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

A few comments, but this is looking quite good! My only concern is if there are tests that are common to multiple transformations, we should centralize them so that if things change in the future we only have to make changes in one place.

Comment on lines 23 to 24
# Check one disjunct for proper contents. Disjunct structure should be
# identical to the version for the inner representation gdp
Copy link
Contributor

Choose a reason for hiding this comment

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

For this and the others that are identical to the inner representation GDP, can we put these methods in the common_tests and just check them in both?

Copy link
Contributor Author

@sadavis1 sadavis1 Feb 22, 2024

Choose a reason for hiding this comment

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

It wouldn't really be appropriate for common_tests, since that file is also used for other transformations that are not inner representation-related. We could do this by moving the methods into a superclass between unittest.TestCase and the transformation test class, but I feel like that's unnecessarily complex to only house two duplicated methods. Well, we could start moving the test method boilerplate into the superclass too, but then it becomes inconsistent with the other transformations that have all their test methods duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe lets not convolute the class structure any more--I agree that that's not worth it, but could we move the methods themselves into functions in common_tests (or make a short common_inner_repn_tests script if you prefer), and just make this method call that function? I do this in GDP a lot: my convention is to name the function check_whatever, have its first argument be the test class (self) and then in the test_whatever method, I just call check_whatever. Sorry to be picky, but maintaining tests in code that could potentially change a lot is really hard, and I think having it be very clear to future us what we expect to be common will be nice. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works. I've moved the methods to a common_inner_repn_tests file.

@sadavis1
Copy link
Contributor Author

Oh... oops. I seem to have done something strange with git. One minute..

@emma58 emma58 self-requested a review February 27, 2024 19:42
Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

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

Looks good!

@emma58 emma58 requested a review from mrmundt April 2, 2024 14:51
@blnicho blnicho requested a review from jsiirola April 2, 2024 19:14
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 96.05263% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 86.55%. Comparing base (69082ac) to head (e954043).

Files Patch % Lines
...o/contrib/piecewise/transform/nested_inner_repn.py 93.67% 5 Missing ⚠️
...b/piecewise/transform/disaggregated_logarithmic.py 98.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3036      +/-   ##
==========================================
- Coverage   88.39%   86.55%   -1.85%     
==========================================
  Files         847      849       +2     
  Lines       95263    95405     +142     
==========================================
- Hits        84212    82582    -1630     
- Misses      11051    12823    +1772     
Flag Coverage Δ
linux ?
osx ?
other 83.83% <96.05%> (-2.70%) ⬇️
win 83.83% <96.05%> (+<0.01%) ⬆️

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.

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.

I think this looks good. I went ahead and changed the one thing that was bothering me (converting int to binary vectors without going through strings)

@blnicho blnicho dismissed mrmundt’s stale review May 8, 2024 18:10

The requested changes have been made

@blnicho blnicho merged commit 12bcecf into Pyomo:main May 8, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants