Adding loopTool to analysis tools #1098

avdudchenko · 2023-08-08T03:10:22Z

Fixes/Resolves:

This is PR includes the complete loopTool that uses current WaterTAP parametr sweep tool.

The PR includes the full loopTool and documentation.
-Support parameter sweeps
-Support differential parameter sweeps

Currently, the loopTool is not setup to run tests in parallel due to ongoing work of adding a parallel manager, this will need to be updated later on.

(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

k1nshuk · 2023-08-09T20:16:31Z

watertap/tools/analysis_tools/loop_tool/loop_tool.py

+        else:
+            return {key: loop_value}
+
+    def setup_multi_processing(self):


We should use the parallel manager to setup parallel computing. It has logic for how to handle concurrent futures or MPI depending on the user specification and conda environment.

This is for the loopTool, as it needs to know if its running with MPI or not for when handling file saving (e.g. getting rank 0), other wise it setups number of subprocess and passes it to parameter sweep tool, which sets up its own parallel management.
Furthermore, number of subprocesses is only auto defined by looptool, if user does not provide a number them self, as it always opts to run in parallel, this is not the case in parameter sweep tool. We can change this behavior in parametersweep tool (e.g. always revert to multi-processing and auto define number of subprocesses based on available resources). Either way is fine, but the loopTool should not handle the logic for how paramterSweep tool handles multiprocessing, it should just instruct it on what should be done.

watertap/tools/analysis_tools/loop_tool/tests/test_expected_diff_directory.yaml

watertap/tools/analysis_tools/loop_tool/tests/test_expected_sweep_directory.yaml

codecov · 2023-08-09T22:19:25Z

Codecov Report

Patch coverage: 93.81% and no project coverage change.

Comparison is base (3b37c9c) 94.92% compared to head (66574b5) 94.93%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1098    +/-   ##
========================================
  Coverage   94.92%   94.93%            
========================================
  Files         331      333     +2     
  Lines       32501    32950   +449     
========================================
+ Hits        30852    31280   +428     
- Misses       1649     1670    +21

Files Changed	Coverage Δ
...tertap/tools/analysis_tools/loop_tool/loop_tool.py	`93.52% <93.52%> (ø)`
...ools/analysis_tools/loop_tool/data_merging_tool.py	`93.93% <93.93%> (ø)`
...ap/tools/parameter_sweep/parameter_sweep_reader.py	`98.90% <96.29%> (-1.10%)`	⬇️
...atertap/tools/parallel/parallel_manager_factory.py	`93.93% <100.00%> (+0.39%)`	⬆️
watertap/tools/parameter_sweep/model_manager.py	`93.54% <100.00%> (+11.29%)`	⬆️
watertap/tools/parameter_sweep/parameter_sweep.py	`96.11% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

PR#1102 adds managment for num of subprocess for ray io when running on cluster

not really needed can be done in place

adam-a-a

Here are some comments from a first pass. I didn't go through all the details just yet but plan to do a second, more thorough pass.

docs/technical_reference/tools/loopTool.rst

adam-a-a · 2023-08-21T14:11:54Z

watertap/tools/parameter_sweep/parameter_sweep_reader.py

+                    "nominal_lb": values["nominal_lb"],
+                    "nominal_ub": values["nominal_ub"],


Shouldn't these point to nominal_lb and nominal_ub based on the logic above regarding diff_mode being precentile or not?

Yes fixed that in next commit.

Co-authored-by: Adam Atia <aatia@keylogic.com>

.github/workflows/mpi4py-test.yml

docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst

watertap/tools/analysis_tools/loop_tool/loop_tool.py

watertap/tools/analysis_tools/loop_tool/tests/test_loop_tool.py

watertap/tools/analysis_tools/loop_tool/loop_tool.py

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

…to loopTool

k1nshuk · 2023-09-07T19:11:47Z

watertap/tools/analysis_tools/loop_tool/tests/test_loop_tool.py

+    # with open("test_expected_diff_directory.yaml", "w") as file:
+    #     documents = yaml.dump(lp.sweep_directory, file)


Suggested change

# with open("test_expected_diff_directory.yaml", "w") as file:

# documents = yaml.dump(lp.sweep_directory, file)

I want to leave these in, so its easy to recreate the test file in the future. (You can't really create it manually)...

k1nshuk

This is already a big PR so it may be fine to make changes in subsequent PRs.

k1nshuk · 2023-09-07T19:17:34Z

watertap/tools/analysis_tools/loop_tool/loop_tool.py

+            outputs[key] = model.find_component(pyo_object)
+        return outputs
+
+    def run_parameter_sweep(self):


Instead of constructing the keyword argument dictionary here and constructing the parameter sweep object, can we call the parameter_sweep function? Same goes for the differential parameter sweep function later in this file

I need to use the ps API directly, as some options are not available in parmenter functions (Which IMO should be depreciated, not sure why we even have that). (such as custom_function and custom_function_kwargs).

lbibl

I didn't find notable issues. Approving at this point. Maybe we count on more testing of them in future's practices.

* merged LoopTool from Analysis * updated diff test * add_diff_fixed_names * Update loopTool.rst * Update index.rst * fixed tests * Fixing file pathing, using abspath(__file__) * fixed covecod for diff_spec, and pathing in test files * switched to useing parallel manager to get mpi config PRwatertap-org/watertap#1102 adds managment for num of subprocess for ray io when running on cluster * simplify mpi check * Remove setup_multi_processing not really needed can be done in place * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * Update docs/technical_reference/tools/loopTool.rst Co-authored-by: Adam Atia <aatia@keylogic.com> * update watertap-org/watertap#1 to match new async setup * updarted tests to reader and small fixes throughout * added additonal tests * Update test_data_merging_tool.py * moved doc to howto as it seems better place for it * Update index.rst * Removed legacy code, added additional tests, added MPI test * Update mpi4py-test.yml * mpi fix try watertap-org/watertap#1 * Fixing MPI test * Fixed MPI rank * Clean up and changes to MPI rank * update doc and added outputs and tests * cleaned up sim_csaes and added key tests for yaml config file * Update mpi4py-test.yml * Update docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * Update watertap/tools/analysis_tools/loop_tool/tests/test_loop_tool.py Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> * updates to loopt tool * Update test_loop_tool.py * Update test_loop_tool.py * Update loop_tool.py --------- Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com> Co-authored-by: bknueven <30801372+bknueven@users.noreply.github.com> Co-authored-by: Adam Atia <aatia@keylogic.com>

avdudchenko added 5 commits August 7, 2023 16:58

merged LoopTool from Analysis

6c2880b

updated diff test

379bf9a

add_diff_fixed_names

7fa857e

Update loopTool.rst

9499d8a

Update index.rst

60cbd2c

avdudchenko requested review from k1nshuk, adam-a-a, TimBartholomew and bknueven August 8, 2023 03:10

avdudchenko and others added 3 commits August 7, 2023 22:25

fixed tests

5569f71

Fixing file pathing, using abspath(__file__)

05d6426

Merge branch 'main' into loopTool

500cce3

k1nshuk requested changes Aug 9, 2023

View reviewed changes

fixed covecod for diff_spec, and pathing in test files

22c9f1e

avdudchenko requested a review from k1nshuk August 10, 2023 04:15

ksbeattie added the Priority:High High Priority Issue or PR label Aug 10, 2023

ksbeattie assigned avdudchenko Aug 10, 2023

avdudchenko and others added 4 commits August 14, 2023 15:41

switched to useing parallel manager to get mpi config

2666df9

PR#1102 adds managment for num of subprocess for ray io when running on cluster

simplify mpi check

dbcb1e0

Remove setup_multi_processing

8720a9c

not really needed can be done in place

Merge branch 'main' into loopTool

943c57e

adam-a-a reviewed Aug 21, 2023

View reviewed changes

avdudchenko and others added 7 commits August 23, 2023 23:39

Merge branch 'main' into loopTool

f85fbb3

Update docs/technical_reference/tools/loopTool.rst

0af609f

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update docs/technical_reference/tools/loopTool.rst

526404e

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update docs/technical_reference/tools/loopTool.rst

36ce3f2

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update docs/technical_reference/tools/loopTool.rst

c72c2f2

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update docs/technical_reference/tools/loopTool.rst

404582d

Co-authored-by: Adam Atia <aatia@keylogic.com>

Update docs/technical_reference/tools/loopTool.rst

e4b285d

Co-authored-by: Adam Atia <aatia@keylogic.com>

cleaned up sim_csaes and added key tests for yaml config file

284c07c

bknueven reviewed Aug 28, 2023

View reviewed changes

.github/workflows/mpi4py-test.yml Outdated Show resolved Hide resolved

avdudchenko and others added 2 commits August 28, 2023 15:04

Update mpi4py-test.yml

9b973ce

Merge branch 'main' into loopTool

28c29f4

k1nshuk requested changes Sep 6, 2023

View reviewed changes

avdudchenko and others added 12 commits September 5, 2023 22:40

Update docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst

58b9ad1

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/loop_tool.py

d65d160

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/loop_tool.py

0229611

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update docs/how_to_guides/how_to_use_loopTool_to_explore_flowsheets.rst

a25ed6c

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/loop_tool.py

918ee55

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/loop_tool.py

9847204

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/loop_tool.py

83fadc8

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

Update watertap/tools/analysis_tools/loop_tool/tests/test_loop_tool.py

bf51ab5

Co-authored-by: Kinshuk Panda <kinshukpanda@gmail.com>

updates to loopt tool

d289c45

Merge branch 'loopTool' of https://github.com/avdudchenko/watertap in…

234ff05

…to loopTool

Update test_loop_tool.py

077abf2

Update test_loop_tool.py

c9e1fd4

k1nshuk reviewed Sep 7, 2023

View reviewed changes

k1nshuk approved these changes Sep 7, 2023

View reviewed changes

Merge branch 'main' into loopTool

66574b5

ksbeattie requested review from hunterbarber, luohezhiming and lbibl and removed request for luohezhiming September 7, 2023 20:28

Merge branch 'main' into loopTool

139ecf5

bknueven approved these changes Sep 8, 2023

View reviewed changes

Update loop_tool.py

dff496f

lbibl approved these changes Sep 12, 2023

View reviewed changes

bknueven merged commit 196dd4b into watertap-org:main Sep 13, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding loopTool to analysis tools #1098

Adding loopTool to analysis tools #1098

avdudchenko commented Aug 8, 2023

k1nshuk Aug 9, 2023

avdudchenko Aug 10, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading

adam-a-a left a comment

adam-a-a Aug 21, 2023

avdudchenko Aug 24, 2023

k1nshuk Sep 7, 2023

avdudchenko Sep 8, 2023

k1nshuk left a comment

k1nshuk Sep 7, 2023

avdudchenko Sep 8, 2023

lbibl left a comment

		"nominal_lb": values["nominal_lb"],
		"nominal_ub": values["nominal_ub"],

		# with open("test_expected_diff_directory.yaml", "w") as file:
		# documents = yaml.dump(lp.sweep_directory, file)

Adding loopTool to analysis tools #1098

Adding loopTool to analysis tools #1098

Conversation

avdudchenko commented Aug 8, 2023

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

k1nshuk Aug 9, 2023

Choose a reason for hiding this comment

avdudchenko Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

adam-a-a left a comment

Choose a reason for hiding this comment

adam-a-a Aug 21, 2023

Choose a reason for hiding this comment

avdudchenko Aug 24, 2023

Choose a reason for hiding this comment

k1nshuk Sep 7, 2023

Choose a reason for hiding this comment

avdudchenko Sep 8, 2023

Choose a reason for hiding this comment

k1nshuk left a comment

Choose a reason for hiding this comment

k1nshuk Sep 7, 2023

Choose a reason for hiding this comment

avdudchenko Sep 8, 2023

Choose a reason for hiding this comment

lbibl left a comment

Choose a reason for hiding this comment

avdudchenko Aug 10, 2023 •

edited

Loading

codecov bot commented Aug 9, 2023 •

edited

Loading