Scaling warning#1658
Conversation
dallan-keylogic
requested review from
MAZamarripa,
agarciadiego,
bpaul4 and
lbianchi-lbl
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1658 +/- ##
================================================
Coverage 77.09% 77.09%
================================================
Files 395 395
Lines 63165 63167 +2
Branches 10370 10371 +1
================================================
+ Hits 48696 48698 +2
Misses 12021 12021
Partials 2448 2448 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| except (AttributeError, KeyError): | ||
| # No scaling factor found, return the default value | ||
| if warning: | ||
| _log.warning(f"Missing scaling factor for {component.name}") |
There was a problem hiding this comment.
Should we make the user aware that he or she can add a default, but preferably, should add an appropriate scaling factor?
There was a problem hiding this comment.
Because of the way the scaling toolbox is set up now, values in the default dictionary will be ignored unless the scaler object specifically calls scale_variable_by_default (see #1648 item 3).
| except (AttributeError, KeyError): | ||
| # No scaling factor found, return the default value | ||
| if warning: | ||
| _log.warning(f"Missing scaling factor for {component.name}") |
There was a problem hiding this comment.
I'm concerned this warning will result in a huge log output for many cases where users don't have scaling, or don't want/need to scale everything. Can we use this error check as a debug step, like a certain level of info reporting but otherwise don't display for every component? The current "missing scaling suffix" warnings can already get very verbose.
There was a problem hiding this comment.
We thought about that, but having the log output is a lesser evil than either 1) having an exception thrown or 2) having the scaling silently fail.
There were two contributing reasons to the log spam using the old tools. First, many times the scaling routines were looking for scaling factors in the wrong places, e.g., looking for scaling factors on an indexed Var when scaling factors had been set for the VarData children of that indexed Var. Now an exception is thrown if the user tries to call get_scaling_factor on an indexed Var. Second, the old scaling tools demanded the user set scaling factors for a ton of variables at the ControlVolume level, such as for heat, mass transfer, extent of reaction, etc. The ControlVolume0DScaler provides a reasonable initial guess for those scaling factors that should work for most use cases. Finally, we're now going to raise an exception if the user doesn't provide default scaling factors for the modular properties, so they'll be forced to provide a bare minimum of scaling.
Ideally, now the warning will be shown only if something has gone seriously wrong with scaling, either because a scaler was written incorrectly or there was no scaler written for the property package in question. We'll see how it works in practice.
One thing I'd like to do is enable the Pytest option that turns warnings into exceptions to catch any issues in the main repository. We still need to 1) write a log filter object to get rid of the Pyomo warnings about elements of a suffix not being exported (so the end user of IDAES doesn't even know Pyomo is logging those warnings) and 2) figure out a way to filter out warnings from the old scaling tools when tests are run.
* error for unnamed expressions * pylint * default is warning=False * make sure expression walker doesn't emit warnings * rescue files from different branch * tests for scaler base get_scaling_factor
5 participants
Summary/Motivation:
Add a warning when trying to get a scaling factor and it does not exist.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: