Pyomo.DoE: Correct A optimality #3803
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. |
There was a problem hiding this comment.
Looks like you replaced every instance of trace with pseudo_trace in these tests. trace is still a valid objective type so I'm curious why you wouldn't want a mix of tests with both options.
There was a problem hiding this comment.
I have not done any testing for the new trace(trace of the covariance matrix). The previous Pyomo.DoE versions that used trace actually meant trace of Fisher Information Matrix (FIM). Now, following Dan's Greybox paper, we are referring to the trace of Fisher Information Matrix (FIM) as pseudo_trace. That's why I have replaced all instances of trace with pseudo_trace. We need new tests for the new trace (trace of the covariance matrix). I plan to do that after the review. Does this answer your question? It may be confusing.
There was a problem hiding this comment.
Thanks for the clarification. I think we should include those tests in this PR before it gets merged.
There was a problem hiding this comment.
I will add tests for the new trace (trace of the covariance matrix). This PR is for initial review to determine whether I need to make any major changes before I proceed with the testing. Thanks for reviewing.
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`
There was a problem hiding this comment.
@blnicho @adowling2 I have created a DoE example for the rooney_biegler experiment, mainly for testing DoE. Where should this live? I can move it to parameter_estimation_example.py and rename the file to rooney_biegler_example.py or something similar. Or are you okay with the current standalone script?
|
@blnicho @adowling2 I have added tests for A-optimality and added tests for the plotting functionality in |
|
@adowling2, this PR is ready for final review. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3803 +/- ##
==========================================
+ Coverage 89.41% 89.46% +0.04%
==========================================
Files 909 907 -2
Lines 105579 105621 +42
==========================================
+ Hits 94408 94493 +85
+ Misses 11171 11128 -43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
smondal13
moved this from Ready for design review
to Ready for final review
in ParmEst & Pyomo.DoE Development
4 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: