-
Notifications
You must be signed in to change notification settings - Fork 562
Pyomo.DoE: Correct A optimality #3803
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
Open
smondal13
wants to merge
29
commits into
Pyomo:main
Choose a base branch
from
smondal13:change-a-optimality
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+993
−121
Open
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
6a75654
changed the definition of A-optimality in `doe/utils.py`
smondal13 4ba1813
used the correct definition of A-optimilty in both utils.py and doe.py
smondal13 5c7462f
changed the label of the colorbar to trace(cov)
smondal13 e3d4833
Merge branch 'Pyomo:main' into change-a-optimality
smondal13 4a50d2a
Adds A-optimlaity objective and constraints with round off improvemen…
smondal13 b5a690b
Add roundoff correction constraint. Without initialization, the MM ex…
smondal13 106781b
Add initialization scheme for L_inv. After adding the initialization …
smondal13 91b0daf
Add trace_FIM in ``compute_FIM_full_factorial()` and change related `…
smondal13 682bf94
Update failing tests in and refactor the test with helper functions …
smondal13 efe4505
Reorder the helper functions' sequence and add comment for better und…
smondal13 31347a9
Raise `ValueError` in the edgecase when `objective_option = 'trace'` …
smondal13 78f0a78
Add pseudo A-optimality heatmap in the doe sensitivity plotting function
smondal13 241d52c
Apply suggestions from code review
smondal13 2eba303
Add both `trace_FIM (pseudo_A_optimality)` and `trace_cov(A-optimality)`
smondal13 51bd8c8
Merge branch 'parmest-consolidate-rooney-biegler' into change-a-optim…
smondal13 76baff0
Add `self.model is None` in `rooney_biegler.py` `get_labeled_model()`…
smondal13 cf2a7d2
Add A-optimality test
smondal13 50a81a6
Add tests for A-optimality in test_doe_build and test_doe_errors.py
smondal13 069aa50
Add test files for the objective trace and the draw_factorial plottin…
smondal13 f671418
Activate t``improve_cholesky_roundoff_error=True`` in the `rooney_bie…
smondal13 785a449
Add `attempt_import` for matplotlib in `test_doe_solve.py`
smondal13 a362b30
Merge branch 'main' into change-a-optimality
smondal13 d4dad65
Add tests for `rooney_biegler/doe_example.py`
smondal13 7371bed
Add corrected test for A-optimality
smondal13 c8d931c
Merge branch 'main' into change-a-optimality
smondal13 b1d980c
Add tests for doe plotting functionality
smondal13 3c430e9
Uncommented the fim_initial.
smondal13 705d83a
Add glob import at the beginning and removes inside function import o…
smondal13 4e11069
Merge branch 'main' into change-a-optimality
blnicho File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Looks like you replaced every instance of
tracewithpseudo_tracein these tests.traceis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not done any testing for the new
trace(trace of the covariance matrix). The previous Pyomo.DoE versions that usedtraceactually meant trace of Fisher Information Matrix (FIM). Now, following Dan's Greybox paper, we are referring to thetrace of Fisher Information Matrix (FIM)aspseudo_trace. That's why I have replaced all instances oftracewithpseudo_trace. We need new tests for the newtrace(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.