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

Handle uninitialized variable in propagate_solution of scaling transformation #3275

Merged
merged 9 commits into from
Aug 20, 2024

Conversation

Robbybp
Copy link
Contributor

@Robbybp Robbybp commented May 28, 2024

Fixes #3273, Fixes #2817

Changes proposed in this PR:

  • If a variable has a value of None in the scaled model, we don't attempt to set its value in the original model

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 May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.56%. Comparing base (5206eee) to head (beb31f6).
Report is 286 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3275   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         877      877           
  Lines       99276    99282    +6     
=======================================
+ Hits        87920    87927    +7     
+ Misses      11356    11355    -1     
Flag Coverage Δ
linux 86.10% <100.00%> (+<0.01%) ⬆️
osx 75.76% <100.00%> (+<0.01%) ⬆️
other 86.59% <100.00%> (+<0.01%) ⬆️
win 83.92% <100.00%> (+<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.

value(scaled_v[k]) / component_scaling_factor_map[scaled_v[k]],
skip_validation=True,
)
if scaled_v[k].value is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Is the correct check here to test None, or to not transfer the values of any stale variables? I am wondering if the correct answer is to only propagate variable values when .stale==False

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 guess I can see three options:

  1. Propagate values in the model, unless they are None (what this PR currently does)
  2. Propagate values in the model, regardless of what they are
  3. Propagate non-stale values in the model

My instinct is that relying on the stale flag can lead to non-intuitive behavior, e.g. if a user sets a value after a solve (or solves via solve_strongly_connected_components), only the last values updated will be propagated. If this is what we decide on, the docstring should be updated to indicate that only the most recent solution is propagated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on @dallan-keylogic's comment on #2817, I've updated this PR to propagate values from the scaled model even if they are None, but to log a warning if None overrides a non-None value.

Copy link
Member

Choose a reason for hiding this comment

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

I think I am OK with this behavior, but if we are always going to propagate the entire model state, I wonder if a better name for this method is propagate_state() (as we are propagating the current state of the model and not necessarily the last thing that came back from the solver).

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 agree that propagate_state, or something like propagate_variable_values, would be a better name, although I'm not convinced it's worth changing.

@Robbybp
Copy link
Contributor Author

Robbybp commented Jun 25, 2024

This should fix #2817 as well.

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 have a minor comment that would improve testing, but that shouldn't necessarily prevent merging.

pyomo/core/tests/transform/test_scaling.py Outdated Show resolved Hide resolved
@blnicho blnicho merged commit b498f6b into Pyomo:main Aug 20, 2024
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
No open projects
4 participants