Simplify _Q_opt in parmest with block scenario structure.#3789
Simplify _Q_opt in parmest with block scenario structure.#3789sscini wants to merge 185 commits into
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 @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 90.10% 90.10% -0.01%
==========================================
Files 904 904
Lines 107113 107110 -3
==========================================
- Hits 96512 96507 -5
- Misses 10601 10603 +2
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:
|
|
@adowling2 @djlaky Should I tag larger team on this? Please review when available. Thanks! |
…ocks" This reverts commit 1aea99f.
… Q_opt-redesign
mrmundt
left a comment
mrmundt
left a comment
There was a problem hiding this comment.
I am confused about the usage of mock in the tests.
|
@mrmundt Please rerun the one failing check. It failed really early. |
mrmundt
left a comment
mrmundt
left a comment
There was a problem hiding this comment.
I have two comments about what look like residual comments / blocks, but they aren't critical to remove.
|
|
||
| if "BootList" in outer_cb_data: | ||
| bootlist = outer_cb_data["BootList"] | ||
| # print("debug in callback: using bootlist=",str(bootlist)) |
There was a problem hiding this comment.
Probably don't need this anymore but not critical to remove
There was a problem hiding this comment.
@mrmundt This appears to be in the deprecated interface. Should this be done in a future PR?
There was a problem hiding this comment.
Real failure-mode gap — fixed in 4964f915. Updated the Tailwind-bump risk mitigation:
MVP: one live kitchen-sink artifact at a stable URL. A bump rebuilds and re-publishes the same URL — every release picks up the new version on next render. We explicitly accept that an old release does NOT preserve prior-version visual fidelity if the upstream bump changes utility semantics; bump policy compensates via a regression-test sweep across representative sites before promoting the new version. This is acceptable because we control all the input (inspector only emits utilities we ship rules for) and a bump is a coordinated platform op.
Post-MVP: per-site compile produces a content-hashed artifact per release; the asset is retained as long as any release references it (retention-immune). The visitor's <link> can fall back to the current stable URL if a versioned URL 404s.
No more 404-unstyled-page failure mode in either era — MVP via stable URL, post-MVP via retention + fallback.
| """ | ||
| try: | ||
| instance = callback(scenario_tree_model, scen_name, node_names) | ||
| except TypeError: # deprecated signature? | ||
| try: | ||
| instance = callback(scen_name, node_names) | ||
| except: | ||
| print("Failed to create instance using callback; TypeError+") | ||
| raise | ||
| except: | ||
| print("Failed to create instance using callback.") | ||
| raise | ||
| """ |
There was a problem hiding this comment.
@mrmundt Similar comment here. As this is in the deprecated interface, should this be done in a future PR?
There was a problem hiding this comment.
Thank you @mrmundt and @slilonfe5. In previous discussions, @blnicho and I agreed to use a separate PR to remove unused code and potentially update the deprecated interface. I will try removing this, but it might lead to new test failures in deprecated. Will happily discuss in the dev meeting today.
8 participants
Fixes # .
Summary/Motivation:
_Q_opt, the main model building and solving function within parmest, is still heavily dependent on MPISPPY, 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: