-
Notifications
You must be signed in to change notification settings - Fork 561
Simplify _Q_opt in parmest with block scenario structure. #3789
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
base: main
Are you sure you want to change the base?
Conversation
|
@djlaky @adowling2 Here are my initial thoughts on the redesign. Please provide feedback on code layout to this point. |
|
Dan and I had a good conversation today about the functional things needed in the redesign, and how to go about adding components to the overall block and sub-models. Working to implement changes, closely related to what is done currently in doe's _generate_scenario_blocks. |
adowling2
left a comment
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.
Nice progress
|
@adowling2 @djlaky Added case for bootstrap, now works with theta_est, theta_est_bootstrap, and theta_est_leaveNout if I replace _Q_opt with _Q_opt_blocks. Please review. I am aware of the duplicated code, but when I attempted to unify them using n_scenarios = len(self.exp_list) or len(bootlist), I am getting an error for bootstrap I am still working out. Would we want to fully transition to using theta_est for obj and theta, and cov_est for covariance and add an error/warning? Or should I make it work to calculate cov if calc_cov=True? Thanks! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3789 +/- ##
==========================================
- Coverage 89.40% 89.31% -0.10%
==========================================
Files 909 909
Lines 105541 105646 +105
==========================================
- Hits 94364 94360 -4
- Misses 11177 11286 +109
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:
|
Fixes # .
Summary/Motivation:
_Q_opt, the main model building and solving function within parmest, is still heavily dependent on MSISPPY, and the code is becoming outdated and difficult to interpret. The goal of this PR is to redesign _Q_opt to work without this dependence, retaining all the current functionality
Changes proposed in this PR:
-_Q_opt redesign, using a block structure, with minimal changes needed to functions that utilize it.
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: