-
Notifications
You must be signed in to change notification settings - Fork 2
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
Remove Block #15
base: doe_review_2
Are you sure you want to change the base?
Remove Block #15
Conversation
@adowling2 I still cannot add reviewers. |
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.
@jialuw96 Here are some comments. The import statements look strange. We can talk about this on Friday.
Once the interface is changed and the examples are updated, we need to write unit tests for new features. For instance, we are changing the interface to the Measurement class. We should write unit tests for that.
pyomo/contrib/doe/doe.py
Outdated
from pyomo.contrib.doe.result import FisherResults, GridSearchResult | ||
#from pyomo.contrib.doe.scenario import Scenario_generator | ||
#from pyomo.contrib.doe.result import FisherResults, GridSearchResult | ||
from scenario import Scenario_generator |
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 is this missing pyomo.contrib.doe
at the beginning of the import statement? Happy to discuss in detail on Friday.
@@ -29,44 +29,45 @@ | |||
import pyomo.environ as pyo | |||
from pyomo.dae import ContinuousSet, DerivativeVar | |||
import numpy as np | |||
from pyomo.contrib.doe import Measurements | |||
from measurements import Measurements |
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 did this import statement change? We can discuss on Friday.
@adowling2 The compute_fim() function is ready for review, both sequential_finite mode and direct_kaug mode. |
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.
@jialuw96 See my comments.
pyomo/contrib/doe/doe.py
Outdated
FIM_analysis = FisherResults(list(self.param.keys()), self.measure, jacobian_info=None, all_jacobian_info=jac, | ||
prior_FIM=prior_in_use, store_FIM=self.FIM_store_name, scale_constant_value=self.scale_constant_value) | ||
# force all design variables in blocks be the same as global design variables | ||
def fix_design1(m,s): |
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 needs to be generalized. Happy to talk about that in our next meeting. One idea is for the user to provide a list of the design variable 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.
We need to think about how to do this in a way that accommodates extra indices.
m.index1 = False | ||
elif model_form == 'dae-index-1': | ||
m.index1 = True | ||
if model_option == "parmest": |
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.
Should this be...
if model_option == "parmest":
...
elif model_option == "global":
else:
if not mod:
raise error here?
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 depends on if we want to create the model by ourself, or let user create model in global mode. Which one you think is better?
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 want to do some error checking to ensure the options passed to model_option
are valid.
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.
You can resolve this discussion thread.
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.
We do need to test these value errors are being thrown.
pyomo/contrib/doe/measurements.py
Outdated
self.flatten_measure_timeset = flatten_measure_timeset | ||
|
||
def _model_measure_name(self): | ||
if self_define_res: |
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 not the interface we discussed. We worked out having an add_measurement
function that the user would call for each measurement and specify indices. This function would then build up the data stored in self.measurement_name
.
pyomo/contrib/doe/measurements.py
Outdated
else: | ||
raise AttributeError("No measurement names provided. Define either self_define_res or measurement_var.") | ||
|
||
def _measure_name(self): | ||
"""Return pyomo string name |
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 doc string needs more explanation. The structure of self.measure_varname and other data are not clear.
pyomo/contrib/doe/measurements.py
Outdated
values are a list of measuring time point. | ||
For e.g., for the kinetics illustrative example, it should be {'C':{'CA':[0,1,..], 'CB':[0,2,...]}, 'k':[0,4,..]}, | ||
so the measurements are C[scenario, 'CA', 0]..., k[scenario, 0].... | ||
self_define_res: a ``list`` of ``string``, containing the measurement variable names with indexs, |
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 should be split into two functions instead of having all of these optional arguments. One function to directly specify the list of all measurement variable names. The section function to build up this list. That will make the documentation and examples much clearer. It will also be easier to test.
pyomo/contrib/doe/result.py
Outdated
@@ -85,7 +85,7 @@ def __init__(self, para_name, measure_object, jacobian_info=None, all_jacobian_i | |||
self.logger.setLevel(level=logging.WARN) | |||
|
|||
|
|||
def calculate_FIM(self, dv_values, result=None): | |||
def calculate_FIM(self, dv_values, black_box_res=True, result=None): |
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.
Doc string needs to be updated to explain black_box_res
option.
@adowling2 Optimization function is ready for review. The discussed class is in specialSet.py. Thank you! |
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.
@jialuw96 See my comments below. Nice work!
for t in m.t: | ||
m.dCdt_rule[z,'CC',t].deactivate() | ||
if block: | ||
for s in range(8): |
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 is this eight hardcoded?
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.
Still need to update.
Use len(m.block)
m.index1 = False | ||
elif model_form == 'dae-index-1': | ||
m.index1 = True | ||
if model_option == "parmest": |
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 want to do some error checking to ensure the options passed to model_option
are valid.
@adowling2 The grid search functions and variances are ready for review. Thank you! |
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.
Please are some comments to discuss today.
pyomo/contrib/doe/doe.py
Outdated
""" | ||
return m.FIM[j,d] == sum(sum(m.jac[z,j,i]*m.jac[z,d,i] for z in m.y_set) for i in m.tmea_set) + m.refele[j, d]*self.fim_scale_constant_value | ||
return m.fim[p,q] == sum(m.jac[p,n]*m.jac[q,n] for n in mod.res) + m.priorFIM[p,q]*self.fim_scale_constant_value |
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.
We still need to discuss this.
J.W.: work in progress
for t in m.t: | ||
m.dCdt_rule[z,'CC',t].deactivate() | ||
if block: | ||
for s in range(8): |
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.
Still need to update.
Use len(m.block)
|
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.
Here are my comments. Next time this is ready to review, tag me in a new post. When you edit an existing comment, I do not always get an update.
|
...and standardize the order of argument processing, plus remove the automatic optimization of f(.)**0 -> 1.
because Pyomo requires Python >= 3.7.
[DOC] Remove instructions for python <= 3.0
mod in doc string
Fixes # .
Summary/Motivation:
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: