Skip to content
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

Merged
merged 18 commits into from
Dec 15, 2023

Conversation

avdudchenko
Copy link
Contributor

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:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

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):
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Dec 7, 2023
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (d2fb122) 94.30% compared to head (3c1c9bc) 94.24%.

Files Patch % Lines
...s/parameter_sweep/paramter_sweep_parallel_utils.py 94.56% 5 Missing ⚠️
...ls/parameter_sweep/parameter_sweep_differential.py 97.43% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@avdudchenko avdudchenko marked this pull request as ready for review December 8, 2023 03:42
Copy link
Contributor

@k1nshuk k1nshuk left a 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.

@avdudchenko avdudchenko requested a review from k1nshuk December 14, 2023 22:54
@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) December 15, 2023 04:23
@lbianchi-lbl lbianchi-lbl merged commit 99c1509 into watertap-org:main Dec 15, 2023
23 checks passed
lbianchi-lbl pushed a commit to watertap-org/parameter-sweep that referenced this pull request May 1, 2024
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants