-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fixes and updates for differential parmater sweep tool #1240
Conversation
added check to ensure model is not reset in diff step
@@ -692,167 +701,9 @@ def parameter_sweep(self, *args, **kwargs): | |||
pass | |||
|
|||
|
|||
class ParameterSweep(_ParameterSweepBase): | |||
class ParameterSweep(_ParameterSweepBase, _ParameterSweepParallelUtils): |
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.
What is going on that we need multiple inheritance here?
Is this just to separate the methods or does some other class need to just inherit from _ParameterSweepBase
?
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.
its so both ParameterSweep and DifferentialParamterSweep class can get access to same underlying parallel functions.
I was thinking of just putting them into _ParamterSweepBase as well, but figured it would be clenaer to store all relevant parallel function in a single place.
We can also have _ParamterSweepBase inhert _ParameterSweepParallelUtils as ParatemterSweep and DifferntialParamterSweep inherit _ParamterSweepBase.
Ultimately, the underlying functions would need to move out of paramtersweep class and either into their own class or _ParmaterSweepBase. We can do what you think is better/cleaner.
fixes disroding, and init issues in param tool
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1240 +/- ##
==========================================
- Coverage 94.30% 94.24% -0.07%
==========================================
Files 359 360 +1
Lines 36261 36284 +23
==========================================
- Hits 34197 34195 -2
- Misses 2064 2089 +25 ☔ View full report in Codecov by Sentry. |
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 good, except adding a warning for the debugging data dir.
watertap/tools/parameter_sweep/tests/test_differential_parameter_sweep.py
Outdated
Show resolved
Hide resolved
…watertap#1240) * fixed paralle implemnetation in diff tool * fixed global aray generation * minor clean up * ensures diff sample is not reset added check to ensure model is not reset in diff step * Fixing differential testing * fixed disorder in multiprocessing idx fixes disroding, and init issues in param tool * fixed jupter notebook * fixed issues in diff_func * added warning about using debugging_data_dir with differential parameter sweep. * set debugging_Data_dir to None in pytests. * run black. --------- Co-authored-by: Kinshuk Panda <[email protected]> Co-authored-by: Kinshuk <[email protected]>
Fixes/Resolves:
This PR fixes differential parameter tool and updates it to comply with current ParamterSweep class usage
(1) Fixes issue with model re-initialization when solving differential sample, resulting in erroneous results
(2) Makes Diff tool compatiable with parallel manager useing MultiProcessing, ConcurrentFutures, and RayIO, ensures stable usage with MPI
(3) Refactors parallel functions, and moves them into parameter_sweep_parallel_utils.py to provide generalized functions for use with parameter sweep and differential sweep class
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)
Summary/Motivation:
Diff tool was essentially broken, this provides critical fixes and ensures compatibility with current parallel infrastructure.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: