Handle uninitialized variable in propagate_solution of scaling transformation#3275
Handle uninitialized variable in propagate_solution of scaling transformation#3275blnicho merged 9 commits intoPyomo:mainfrom
propagate_solution of scaling transformation#3275Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| value(scaled_v[k]) / component_scaling_factor_map[scaled_v[k]], | ||
| skip_validation=True, | ||
| ) | ||
| if scaled_v[k].value is not None: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I guess I can see three options:
- Propagate values in the model, unless they are None (what this PR currently does)
- Propagate values in the model, regardless of what they are
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
This should fix #2817 as well. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
I have a minor comment that would improve testing, but that shouldn't necessarily prevent merging.
4 participants
Fixes #3273, Fixes #2817
Changes proposed in this PR:
Nonein the scaled model, we don't attempt to set its value in the original modelLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: