Incompatibility between `set_scaling_from_default()` and generic reaction parameter data
Closed this issue · 2 comments
While experimenting with using the set_scaling_from_default() method to add scaling to a model that includes a reactor, I observed the following AttributeError:
AttributeError: 'ScalarBlock' object has no attribute 'get_default_scaling'
I ran set_scaling_from_default on my entire model, so it descended into every block in turn. Tracing the error back to its source, it appears that the problem is in how the GenericReactionParameterData class instantiates the individual reaction blocks, named "reaction_{idx}".
These reaction blocks are created as generic Pyomo Block objects, which does not have the get_default_scaling method defined. Instead, they should be created as ProcessBaseBlock objects, or something derived from that.
Looks like an easy 3-line fix: 2 lines to update the imports and 1 line to change the pyomo Block to an idaes ProcessBaseBlock. Happy to submit a PR if needed.
@jasonmbray Thanks for bring this to our attention. As you noted, the issue is that get_default_scaling assumes that all Blocks in the model will have the default_scaling attribute but that is not always true. However, the fix here is not to force every Block to have this (as this is impossible to achieve in Python), but instead to have the get_default_scaling function handle the absence of this gracefully.
I.e. at line 1181 it needs to have a try/except:
parent = component.parent_block()
try:
dsf = parent.get_default_scaling(
component.parent_component().local_name, index=component.index()
)
except AttributeError:
dsf = None
This way the function will handle cases where the Block does not have the get_default_scaling method (for whatever reason). If you would like to open a PR to do this, it would be appreciated. You may also wish to log a warning message in the except branch to say that a Block was encountered that did not define default scaling factors.
However, it is also worth noting here that we intend to deprecate most of the current scaling tools in the near future and replace them with more transparent methods (as the current system is quite opaque and even many of us do not know how it all works). This function would likely be among the ones that is deprecated, as it is very difficult to assign good "defaults" for scaling factors as every application is different.
Sorry for the delay. I had to work through some approvals with my employer and set up a separate github account. I've made the change as recommended and created PR #1400 for you all to review.