Conversation
…ample did not solve to the desired point using A-optimality. However, when initialized to small value `1e-5`, it solved to the optimal point.
…scheme, the MM model solved without initialization from outside.
…`doe/tests`` files to reflect the changes.
…n `doe/test_utils.py`
…erstanding in ``doe/test_utils.py``
…and `_Cholesky_option = False`. Add flag for functions are the only used in `trace` calculation.
|
@adowling2 @blnicho This PR is ready for initial review. |
Replace `model` with `m` in `cholesky_LLinv_imp()` Co-authored-by: Bethany Nicholson <[email protected]>
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
@smondal13 thanks for addressing my initial review comments. I think these changes look reasonable so far but would like to see tests for the corrected A-optimality before I'll approve the PR.
… and add `doe_example..py` based on the rooney_biegler.py
…gler/doe_example.py`
adowling2
left a comment
adowling2
left a comment
There was a problem hiding this comment.
I have some concerns about how this testing framework can scale to all of the variants of Pyomo.DoE that we want to try, including:
- central finite difference versus symbolic differentiation
- GreyBox: A, D, E, ME optimality
- Cholesky: A, D
We do not need to add all of these variants in this PR. But I think we need to ensure this PR sets us up to easily add these extensions in other PRs. This is important because I could see multiple parallel PRs to add these features that will all be building on this testing foundation.
|
|
||
| Parameters | ||
| ---------- | ||
| optimize_experiment_A : bool, optional |
There was a problem hiding this comment.
This does not seem very general... instead, do we want to support A, D, E, ME, pseudo A, ... objectives?
There was a problem hiding this comment.
Do you mean that we pass an argument, e.g., objective="trace" / "pesudo_trace"... etc in run_rooney_biegler_doe()` ? I think that is a very good idea.
There was a problem hiding this comment.
Yes, unless there are significant benefits to doing it as a single function (sequentially). If that is the case, you could make objectives the input and support either a single string or a list of strings.
There was a problem hiding this comment.
I have added the objective and the gradient/sensitivity computation option. Maybe symbolic can be added there when symbolic is merged. And solving the example for only one type of objective is fine.
def run_rooney_biegler_doe(
sensitivity_formula="central",
optimization_objective="determinant",
improve_cholesky_roundoff_error=False,
compute_FIM_full_factorial=False,
draw_factorial_figure=False,
design_range={'hour': [0, 10, 40]},
tee=False,
print_output=False,
):
| fd_method = "central" | ||
| obj_used = "trace" | ||
|
|
||
| experiment = run_rooney_biegler_doe()["experiment"] |
There was a problem hiding this comment.
Can we do a lightweight version of FullReactorExperiment, perhaps with pseudo A opt
…sitivity_formula parameters; update tests accordingly
adowling2
left a comment
adowling2
left a comment
There was a problem hiding this comment.
I think this is ready to merge. Where are the changes to the documentation that explain A-optimality versus pseudo-A-optimality? Are they in this PR or a different PR?
| fd_method = "central" | ||
| obj_used = "trace" | ||
|
|
||
| experiment = run_rooney_biegler_doe()["experiment"] |
|
@smondal13 Please also proof read the website version here: https://pyomo--3812.org.readthedocs.build/en/3812/explanation/analysis/doe/doe.html |
|
@smondal13, I think we should update the workflow figure to match our tutorial material: https://dowlinglab.github.io/pyomo-doe/Readme.html |
Added the workshop figure and proofread it. Had some minor corrections |
blnicho
left a comment
blnicho
left a comment
There was a problem hiding this comment.
@smondal13 I think this is really close. I found a few more things in the example which should be straightforward to fix.
github-project-automation
bot
moved this from Review In Progress
to Reviewer Approved
in Pyomo 6.10
github-project-automation
bot
moved this from Ready for final review
to Done
in ParmEst & Pyomo.DoE Development
7 participants
Fixes
Summary/Motivation:
In the old definition of A-optimality (we will call it pseudo A-optimality from now on), DoE computed the trace of the Fisher Information Matrix (FIM), as few studies used that definition. However, that is not correct, and new literature mentions the trace of the inverse of the FIM as A-optimality. This PR has adopted this new definition of A-optimality in DoE.
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: