-
Notifications
You must be signed in to change notification settings - Fork 505
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
Parmest update of util convert_params_to_vars. #3339
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good, but there is a big performance bug that should be addressed.
indexed_param_CUIDs += [ | ||
ComponentUID(theta_obj) for _, theta_obj in theta_object.items() | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more clear / more efficient to use list's extend()
and dict's values()
:
indexed_param_CUIDs.extend(
ComponentUID(theta_obj) for theta_obj in theta_object.values()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as suggested.
var_theta_objects = [ | ||
var_theta_obj for _, var_theta_obj in theta_var_object.items() | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are only using the values from a dict, it is more clear / more efficient to use values()
:
var_theta_objects = list(var_theta_obj.values())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as suggested.
|
||
# Convert Params to Vars in Expressions | ||
for expr in model.component_data_objects(pyo.Expression): | ||
if expr.active and any( | ||
v.name in param_names for v in identify_mutable_parameters(expr) | ||
ComponentUID(v) in param_CUIDs for v in identify_mutable_parameters(expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very inefficient (quadratic in the number of params). If you are going to test membership, param_CUIDs
needs to be a set / dict / mapping object (so that lookups are O(1) and not O(n)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted list to set for the lookups at the end of the function.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
========================================
Coverage 88.52% 88.53%
========================================
Files 868 868
Lines 98436 98771 +335
========================================
+ Hits 87144 87450 +306
- Misses 11292 11321 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still needs some work. There are some bugs in this function when params to be converted to vars live on blocks. This PR didn't necessarily introduce the bugs but I think we should fix them while we're overhauling this function. Here is a small example demonstrating one of the bugs:
from pyomo.environ import *
from pyomo.contrib.parmest.utils import convert_params_to_vars
m = ConcreteModel()
m.p1 = Param(initialize=1, mutable=True)
m.b = Block()
m.b.p2 = Param(initialize=2, mutable=True)
param_cuids = [ComponentUID(m.p1), ComponentUID(m.b.p2)]
m2 = convert_params_to_vars(m, param_cuids)
this fails with an AttributeError
:
AttributeError: 'ScalarParam' object has no attribute 'unfix'
pyomo/contrib/parmest/parmest.py
Outdated
@@ -440,8 +440,8 @@ def TotalCost_rule(model): | |||
) | |||
|
|||
# Convert theta Params to Vars, and unfix theta Vars | |||
theta_names = [k.name for k, v in model.unknown_parameters.items()] | |||
parmest_model = utils.convert_params_to_vars(model, theta_names, fix_vars=False) | |||
theta_CUIDs = [v for k, v in model.unknown_parameters.items()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theta_CUIDs = [v for k, v in model.unknown_parameters.items()] | |
theta_CUIDs = list(model.unknown_parameters.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
@@ -46,12 +50,12 @@ def test_convert_param_to_var(self): | |||
exp = ReactorDesignExperiment(data, 0) | |||
instance = exp.get_labeled_model() | |||
|
|||
theta_names = ['k1', 'k2', 'k3'] | |||
param_CUIDs = [v for k, v in instance.unknown_parameters.items()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
param_CUIDs = [v for k, v in instance.unknown_parameters.items()] | |
param_CUIDs = list(instance.unknown_parameters.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
|
||
# get indexed Params | ||
param_theta_objects = [theta_obj for _, theta_obj in theta_object.items()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
param_theta_objects = [theta_obj for _, theta_obj in theta_object.items()] | |
param_theta_objects = list(theta_object.values()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
param_CUIDs = indexed_param_CUIDs | ||
|
||
# convert to a set for look up efficiency | ||
param_CUIDs_set = set(param_CUIDs) | ||
|
||
# Convert Params to Vars in Expressions | ||
for expr in model.component_data_objects(pyo.Expression): | ||
if expr.active and any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expression
components are not "activatable" so this logic can be simplified to:
if any(ComponentUID(v) in param_CUIDs_set
for v in identify_mutable_parameters(expr)
):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
|
||
# Param | ||
if theta_object.is_parameter_type(): | ||
|
||
# Delete Param, add Var | ||
vals = theta_object.extract_values() | ||
model.del_component(theta_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work on hierarchical models. I think we need to add more tests for edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed this and added a test.
param_names = indexed_param_names | ||
# Update the list of param_CUIDs if the parameters were indexed | ||
if len(indexed_param_CUIDs) > 0: | ||
param_CUIDs = indexed_param_CUIDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we replace param_CUIDs
here instead of extending it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to replace it because it might not have the indexed variables, only the main variable. For example "theta" (param_CUIDs) vs. "theta[asymptote]" and "theta[rate_constant]" (indexed_param_CUIDs).
# get indexed Params | ||
param_theta_objects = [theta_obj for _, theta_obj in theta_object.items()] | ||
|
||
# get indexed Param names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# get indexed Param names | |
# get indexed Param CUIDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Fixes # .
Summary/Motivation:
This is step 1 of a set of internal changes to parmest related to issue:
#3252
Changes proposed in this PR:
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: