Minor improvements to scaling API#1507
Conversation
andrewlee94
requested review from
bpaul4,
dallan-keylogic and
lbianchi-lbl
as code owners
andrewlee94
requested review from
adam-a-a,
agarciadiego,
dallan-keylogic and
radhakrishnatg
and removed request for
bpaul4,
dallan-keylogic and
lbianchi-lbl
andrewlee94
added
enhancement
andrewlee94
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1507 +/- ##
==========================================
- Coverage 76.99% 76.98% -0.01%
==========================================
Files 382 382
Lines 61993 61992 -1
Branches 10146 10146
==========================================
- Hits 47733 47727 -6
- Misses 11852 11858 +6
+ Partials 2408 2407 -1 ☔ View full report in Codecov by Sentry. |
| # Call the class to create an instance | ||
| scaler = scaler() | ||
| _log.debug(f"Using user-defined Scaler for {model}.{submodel}.") | ||
| _log.debug(f"Using user-defined Scaler for {submodel}.") |
There was a problem hiding this comment.
Is this going to give human readable output, or is it going to be some Pyomo thing in brackets?
Consider using submodel.name
There was a problem hiding this comment.
It will end up being submodel.name - it is not obvious but the f-string magic casts it to a string.
agarciadiego
left a comment
agarciadiego
left a comment
There was a problem hiding this comment.
Looks good to me. Only a question about future maintenance in error messages but this should not stop merging.
| "No default Scaler set for unknown.b. Cannot call dummy_method." | ||
| in caplog.text | ||
| ) | ||
| assert "No default Scaler set for b. Cannot call dummy_method." in caplog.text |
There was a problem hiding this comment.
I wonder if only referencing dummy_method in error message may confuse maintainer in the future
There was a problem hiding this comment.
Do you have a suggestion for a better message?
There was a problem hiding this comment.
"No default Scaler set for b. Cannot call dummy_method created for testing purposes."
There was a problem hiding this comment.
The message is generated dynamically using the name of the method the user asked for. In this test case, the method I asked for was named "dummy_method", but a user would see the name of the method they specified.
There was a problem hiding this comment.
Then I think it's fine
4 participants
Fixes None
Summary/Motivation:
Based on initial demonstrations of the new scaling tools, a few minor improvements have been suggested.
Changes proposed in this PR:
call_submodel_scaler_methodto take submodel object instead of string name.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: