Fixes issue for saving model files with missing keys #1151
Conversation
avdudchenko
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
==========================================
+ Coverage 94.93% 94.95% +0.01%
==========================================
Files 343 343
Lines 34794 34809 +15
==========================================
+ Hits 33033 33052 +19
+ Misses 1761 1757 -4
☔ View full report in Codecov by Sentry. |
|
The changes look good to me so far. I can approve it once its out of draft. |
|
Based on the codecov check, it looks like you just need to add some testing. |
… into ps-tool-fixes
|
Looks like the tests now pass and are MPI safe. The test do need min 2 parallel workers to properly test the catches. Fails MacOS test as the objective results are not quiet right (1e-3 precision by looks of it...) Should be good to go/merge. This also includes some additional fixes that were caught during testing/development. |
| def test_parameter_sweep_optimize_with_added_var(self, model, tmp_path): | ||
| # this will run a solve function that adds a variable but only in some | ||
| # of the solves. | ||
| """THIS TEST IS DESIGNED FOR 2 Parallel workers!!!!!!""" |
There was a problem hiding this comment.
What would be needed to make this test work with an arbitrary number of workers? This could be dealt with in a subsequent PR.
There was a problem hiding this comment.
Its a minum number - if you have 1 worker then we don't test for missing keys. I think we should really just change how we store results from each solve (e.g instead of storing all results in arrays inside a result dictionory, we just store the value for each parameter in a dict and store an array of dicts--not sure how MPI safe that is but that would solve this issue. - this would be same as async multiprocessing or ray implementation, where we split parmans into size of 1...
Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>
|
@avdudchenko still for Dec? |
Would be nice, just needs one more review (@bknueven plz) and otherwise its good to go? |
There's still a failing test :( |
…tertap#1151) * Fixes issue when key dont exist * Fixes issue with empty h5 files * added test for missing key fix and couple other small fixes. * Fjxes to mpit testing * Update watertap/tools/parameter_sweep/tests/test_parameter_sweep.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * mac fix * Update test_parameter_sweep.py --------- Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>
Fixes/Resolves:
This will catch when a result is returned with missing or inactive variables. This can occur when a solve fails or the initialization process fails without re-activating the variable first. Or where some variables will be deactivated by user based on solved condition.
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)
Summary/Motivation:
When working with NN's I build the NNs on model tree, but deactivate them. they only end up being reactivated if solve is successful, so if it fails, we will return a result with missing keys. This update will attempt to catch missing keys and rebuild them.
Might not be the best implementation.
needs tests
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: