Migrate PROCESS solver to new interface #1146

hsaunders1904 · 2022-06-14T12:21:21Z

Linked Issues

Partially addresses #956

Description

Re-writes the PROCESS solver with the new solver API. Also added are tests that do not require PROCESS being installed, so the tests can run in the CI.

There are a couple of changes to the SolverABC class - notably, use of ParameterFrames, and the run_mode_cls abstract property. I intend to update the solver example to reflect this.

After this has been merged, the next step is to rebase hsaunders1904/956_move_existing_solvers_to_new_interface and quickly update the Balance of Plant solver that went in recently. Also update the example mentioned above.

Interface Changes

PROCESS solver moved to new API.

Checklist

I confirm that I have completed the following checks:

Tests run locally and pass pytest tests --reactor
Code quality checks run locally and pass flake8 and black .
Documentation built locally and checked sphinx-build -W documentation/source documentation/build

* ✨ process species additions * 🎨 improvements spurred by comments * ✅ add a test * 🐛 pass if process not installed * 🐛 pass if process not installed * Update bluemira/codes/process/api.py Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com> Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com>

This logging is more consistent with what's in PROCESS, solver and more specific about what's happening.

Parameters whose values contained a trailing comma could not be read by PROCESS. The file format is no longer supported by PROCESS, so we no longer support it either. Remove the trailing commas so the IN.DAT file is usable in tests.

This was causing the patched function being called in subsequent tests! Whoops

Mock out the PROCESS file reader so that the Teardown class's logic can be testing without PROCESS being installed.

PROCESS does not necessarily output all the values we need for a radial build. There are certainly some MFiles which are missing some parameters. Catch these cases and raise a better error.

The below was not needed. The files are not required to run the solver.

This will cause an error later on anyway, so the check is not needed.

One of the tests was passing, but because an error of a similar type was caught. Check the error message to make sure the right error is thrown.

This allows us to access the available run modes for a solver, without prior knowledge of which Solver class it is.

codecov-commenter · 2022-06-14T12:42:08Z

Codecov Report

❗ No coverage uploaded for pull request base (hsaunders1904/956_move_existing_solvers_to_new_interface@44c3407). Click here to learn what that means.
The diff coverage is n/a.

@@                                     Coverage Diff                                     @@
##             hsaunders1904/956_move_existing_solvers_to_new_interface    #1146   +/-   ##
===========================================================================================
  Coverage                                                            ?   75.22%           
===========================================================================================
  Files                                                               ?      185           
  Lines                                                               ?    21655           
  Branches                                                            ?        0           
===========================================================================================
  Hits                                                                ?    16289           
  Misses                                                              ?     5366           
  Partials                                                            ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44c3407...a5f509e. Read the comment docs.

The look_and_feel.print_banner test fails if the git branch is too long. The long name caused a new line in the banner, which made the asserted banner length incorrect.

These had been removed as part of the refactor of the PROCESS API.

The mock was overwriting the PROCESS module, instead of patching it. The effect of this was causing a subsequent test to fail.

je-cook

Looks good I have nothing major to say. I will have a little run of the example just to get a feel for it

bluemira/codes/process/_teardown.py

je-cook · 2022-06-14T15:40:10Z

bluemira/codes/process/_teardown.py

+        self.rebuild_RB_dict()
+
+    def rebuild_RB_dict(self):  # noqa :N802
+        """


Just a comment to flag that at some point we should try and remove the need for this, there is an issue somewhere about it I think can't find where. Probably should try and remove the majority of this wrapper when process finishes their api

Is it this one #722?

I think it was a combination of that and #164 I was thinking of

bluemira/codes/process/_solver.py

sonarcloud · 2022-06-15T14:34:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
28 Code Smells

No Coverage information
0.0% Duplication

* Process api additions (#1055) * ✨ process species additions * 🎨 improvements spurred by comments * ✅ add a test * 🐛 pass if process not installed * 🐛 pass if process not installed * Update bluemira/codes/process/api.py Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com> Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com> * Fix PROCESS enum value typo CYRO -> CRYO * Add refactored Setup class for PROCESS solver * Add refactored Run class for PROCESS solver * Remove unused attribute in test class * Clean up logging in plasmod Run class This logging is more consistent with what's in PROCESS, solver and more specific about what's happening. * Write tests ahead of PROCESS Teardown refactor * Make PROCESS solver's Teardown tests pass * Add refactored PROCESS solver class * Remove old PROCESS solver implementation * Fix typo in test that was causing it to pass * Correct docstring in PROCESS solver * Remove trailing commas from PROCESS test IN.DAT Parameters whose values contained a trailing comma could not be read by PROCESS. The file format is no longer supported by PROCESS, so we no longer support it either. Remove the trailing commas so the IN.DAT file is usable in tests. * Fix PROCESS test not stopping mock.patch This was causing the patched function being called in subsequent tests! Whoops * Add tests for PROCESS run modes * Fix some find-replace errors * Make solver's Teardown testable without PROCESS Mock out the PROCESS file reader so that the Teardown class's logic can be testing without PROCESS being installed. * Catch case where PROCESS result missing param PROCESS does not necessarily output all the values we need for a radial build. There are certainly some MFiles which are missing some parameters. Catch these cases and raise a better error. * Resolve PROCESS solver TODO The below was not needed. The files are not required to run the solver. * Fix unused variable in PROCESS solver * Remove check for empty file This will cause an error later on anyway, so the check is not needed. * Fix misbehaving PROCESS tests One of the tests was passing, but because an error of a similar type was caught. Check the error message to make sure the right error is thrown. * Add missing docstrings to PROCESS Teardown * Remove unused constant in test * Replace old codes.interface module * Restore 'plot_radial_build' to PROCESS API * Add run_mode_cls property to Solver This allows us to access the available run modes for a solver, without prior knowledge of which Solver class it is. * Update codes.wrapper to use new Solver interface * Add __all__ imports to codes.plasmod * Use run and read directories in PROCESS Teardown * Fix file paths in PROCESS system tests * Update codes.interface tests * Add some missing docstrings in PROCESS solver * Fix flake8 warning in base.design * Fix test that fails for long git branch names The look_and_feel.print_banner test fails if the git branch is too long. The long name caused a new line in the banner, which made the asserted banner length incorrect. * Restore species methods to PROCESS solver These had been removed as part of the refactor of the PROCESS API. * Fix mocking of PROCESS module The mock was overwriting the PROCESS module, instead of patching it. The effect of this was causing a subsequent test to fail. * Remove superfluous comment * Use walrus operator to summarise if statement * Remove unused imports Co-authored-by: je-cook <81617086+je-cook@users.noreply.github.com>

* Move plasmod solver to new interface (#1054) * Refactor test helper into new module * Use ParameterFrame not Dict in Task constructor * Start on Plasmod interface refactor - Setup * Add plasmod Run task The run_subprocess function has been moved to a separate utility. This allows us to mock our own `run_subprocess` function in tests, and not worry about if we want to change the implementation of how we run our shell commands. E.g., if we wanted to start using os.system rather than subprocess.Popen, for whatever reason. * Refactor plasmod solver interface's Teardown * Fix bug in run_subprocess + add test reminder * Fix param mappings in refactored Plasmod interface * Fix dataclass attribute/method ordering Apparently dataclass attributes need to be declared first for the corresponding kwarg to be added to the constructor. Declare the methods after the attributes to fix this. * Add missing Plasmod Teardown tests * Remove extraneous dict init in plasmod.params * Add refactored plasmod Solver class * Add end-to-end test for plasmod solver * Add 'cwol' to PlasmodOutput dataclass * Add test for error on bad Plasmod result flag * Add test for writing PlasmodInput params * Convert PlasmodInput models to enums on init * Create new PlasmodInputs on Solver.update_inputs This allows for the post-init processing to do its magic. * Fix typing in plasmod MODEL_MAP * Replace old plasmod Solver with refactored * Add warning if plasmod output param has no value Adds back change made in #1009 after refactor. * Split up large files in plasmod.solver * Allow using enum name to specify plasmod models * Warn when using unrecognised inputs in plasmod * Add get_profile back into plasmod.Solver This was temporarily removed as part of a refactor. * Add get_profiles method back into plasmod.Solver This had been temporarily removed during a refactor. * Add 'scalar_outputs' method to plasmod.Solver This replaces the previous 'get_raw_variables' function. Using a structure as an output means that users know exactly what attributes are allowed. IDEs can then make suggestions about attributes, and also spot typos that wouldn't be spotted using strings. * Allow param edits between calls of plasmod solver * Put modify_mappings back into plasmod solver * Flesh out plasmod Task constructor docstrings * Update plasmod example to use new solver interface * Add CodesSolver base class This is intended to replace the FileProgramInterface class, as part of the refactor of the solvers interface. * Move plasmod.Solver functionality to base class * Extract plasmod.Solver general setup to base class * Re-add 'jiter' as plasmod output parameter This had been lost in a refactor. * Separate profiles and scalars in plasmod outputs * Move plasmod exit code check to Teardown task This makes more sense than throwing when reading the outputs. * Undo change in plasmod example's transport model This was an accidental change made during a refactor. * Change to a better method in CodesTeardown * Use plasmod.Profiles enums in Sovler.get_profile This is in-line with the effort towards: "All text based choices should be enumified" #1044 * Remove resolved TODO * Write tests to resolve TODO * Resolve flake8 warnings * Resolve ambiguity in plasmod solver Setup var name This attempts to resolve the ambiguity around the 'input_file' variable name. * Update plasmod solver directory structure This is more consistent with the structure in PROCESS's solver * Fix plasmod example's get_profile usage * Fix sphinx build * Fix file path in flake8 ignores I'd forgotten to update the file path after moving the file. * Fix import in tests * Simplify import in plasmod example * Add missing type hint in CodesTask._run_subprocess * Add Optional to typing * Add better typing + docstrings to udpate_inputs * Migrate PROCESS solver to new interface (#1146) * Process api additions (#1055) * ✨ process species additions * 🎨 improvements spurred by comments * ✅ add a test * 🐛 pass if process not installed * 🐛 pass if process not installed * Update bluemira/codes/process/api.py Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com> Co-authored-by: Harry Saunders <33317174+hsaunders1904@users.noreply.github.com> * Fix PROCESS enum value typo CYRO -> CRYO * Add refactored Setup class for PROCESS solver * Add refactored Run class for PROCESS solver * Remove unused attribute in test class * Clean up logging in plasmod Run class This logging is more consistent with what's in PROCESS, solver and more specific about what's happening. * Write tests ahead of PROCESS Teardown refactor * Make PROCESS solver's Teardown tests pass * Add refactored PROCESS solver class * Remove old PROCESS solver implementation * Fix typo in test that was causing it to pass * Correct docstring in PROCESS solver * Remove trailing commas from PROCESS test IN.DAT Parameters whose values contained a trailing comma could not be read by PROCESS. The file format is no longer supported by PROCESS, so we no longer support it either. Remove the trailing commas so the IN.DAT file is usable in tests. * Fix PROCESS test not stopping mock.patch This was causing the patched function being called in subsequent tests! Whoops * Add tests for PROCESS run modes * Fix some find-replace errors * Make solver's Teardown testable without PROCESS Mock out the PROCESS file reader so that the Teardown class's logic can be testing without PROCESS being installed. * Catch case where PROCESS result missing param PROCESS does not necessarily output all the values we need for a radial build. There are certainly some MFiles which are missing some parameters. Catch these cases and raise a better error. * Resolve PROCESS solver TODO The below was not needed. The files are not required to run the solver. * Fix unused variable in PROCESS solver * Remove check for empty file This will cause an error later on anyway, so the check is not needed. * Fix misbehaving PROCESS tests One of the tests was passing, but because an error of a similar type was caught. Check the error message to make sure the right error is thrown. * Add missing docstrings to PROCESS Teardown * Remove unused constant in test * Replace old codes.interface module * Restore 'plot_radial_build' to PROCESS API * Add run_mode_cls property to Solver This allows us to access the available run modes for a solver, without prior knowledge of which Solver class it is. * Update codes.wrapper to use new Solver interface * Add __all__ imports to codes.plasmod * Use run and read directories in PROCESS Teardown * Fix file paths in PROCESS system tests * Update codes.interface tests * Add some missing docstrings in PROCESS solver * Fix flake8 warning in base.design * Fix test that fails for long git branch names The look_and_feel.print_banner test fails if the git branch is too long. The long name caused a new line in the banner, which made the asserted banner length incorrect. * Restore species methods to PROCESS solver These had been removed as part of the refactor of the PROCESS API. * Fix mocking of PROCESS module The mock was overwriting the PROCESS module, instead of patching it. The effect of this was causing a subsequent test to fail. * Remove superfluous comment * Use walrus operator to summarise if statement * Remove unused imports Co-authored-by: je-cook <81617086+je-cook@users.noreply.github.com> * Update power cycle solver for new api (#1167) * Update EUDEMO power cycle solver for new interface * Remove erroneously restored file after rebase A bad rebase restored a previously deleted file. Re-delete it * Update Solver API example (#1169) * Update Solver API example Updates the example to use a ParameterFrame. * Update PROCESS solver example * Tidy solvers with walrus operator * Fix quotation marks in PROCESS solver warning Co-authored-by: je-cook <81617086+je-cook@users.noreply.github.com>

je-cook and others added 30 commits June 8, 2022 16:39

Fix PROCESS enum value typo CYRO -> CRYO

8526ec0

Add refactored Setup class for PROCESS solver

7f5cfae

Add refactored Run class for PROCESS solver

ee5caf9

Remove unused attribute in test class

5f7287b

Clean up logging in plasmod Run class

9652aaf

This logging is more consistent with what's in PROCESS, solver and more specific about what's happening.

Write tests ahead of PROCESS Teardown refactor

9c551bc

Make PROCESS solver's Teardown tests pass

dd0e4c6

Add refactored PROCESS solver class

916941c

Remove old PROCESS solver implementation

80ece61

Fix typo in test that was causing it to pass

d1875d7

Correct docstring in PROCESS solver

869ed71

Remove trailing commas from PROCESS test IN.DAT

8d9c4f2

Parameters whose values contained a trailing comma could not be read by PROCESS. The file format is no longer supported by PROCESS, so we no longer support it either. Remove the trailing commas so the IN.DAT file is usable in tests.

Fix PROCESS test not stopping mock.patch

fe8f79d

This was causing the patched function being called in subsequent tests! Whoops

Add tests for PROCESS run modes

14ae359

Fix some find-replace errors

d296697

Make solver's Teardown testable without PROCESS

8c59493

Mock out the PROCESS file reader so that the Teardown class's logic can be testing without PROCESS being installed.

Catch case where PROCESS result missing param

8159c7b

PROCESS does not necessarily output all the values we need for a radial build. There are certainly some MFiles which are missing some parameters. Catch these cases and raise a better error.

Resolve PROCESS solver TODO

efcbedb

The below was not needed. The files are not required to run the solver.

Fix unused variable in PROCESS solver

5b5d55b

Remove check for empty file

233f2d6

This will cause an error later on anyway, so the check is not needed.

Fix misbehaving PROCESS tests

0c8cfb2

One of the tests was passing, but because an error of a similar type was caught. Check the error message to make sure the right error is thrown.

Add missing docstrings to PROCESS Teardown

32763ea

Remove unused constant in test

9ff7a27

Replace old codes.interface module

fe587c8

Restore 'plot_radial_build' to PROCESS API

a46cf66

Add run_mode_cls property to Solver

54ee226

This allows us to access the available run modes for a solver, without prior knowledge of which Solver class it is.

Update codes.wrapper to use new Solver interface

b3caae5

Add __all__ imports to codes.plasmod

1464733

Use run and read directories in PROCESS Teardown

8d88b9a

hsaunders1904 added 4 commits June 14, 2022 09:45

Fix file paths in PROCESS system tests

e0e1730

Update codes.interface tests

11f4106

Add some missing docstrings in PROCESS solver

42b7291

Fix flake8 warning in base.design

c981312

hsaunders1904 assigned je-cook Jun 14, 2022

hsaunders1904 requested review from a team as code owners June 14, 2022 12:21

hsaunders1904 added 3 commits June 14, 2022 13:57

Fix test that fails for long git branch names

efc4fe8

The look_and_feel.print_banner test fails if the git branch is too long. The long name caused a new line in the banner, which made the asserted banner length incorrect.

Restore species methods to PROCESS solver

dd8cd82

These had been removed as part of the refactor of the PROCESS API.

Fix mocking of PROCESS module

6678a87

The mock was overwriting the PROCESS module, instead of patching it. The effect of this was causing a subsequent test to fail.

je-cook approved these changes Jun 15, 2022

View reviewed changes

hsaunders1904 added 3 commits June 15, 2022 15:22

Remove superfluous comment

711f9ee

Use walrus operator to summarise if statement

d6f58c7

Remove unused imports

a5f509e

hsaunders1904 merged commit a2857f6 into hsaunders1904/956_move_existing_solvers_to_new_interface Jun 15, 2022

hsaunders1904 deleted the hsaunders1904/956_migrate_process_solver_to_new_interface branch June 15, 2022 15:33

hsaunders1904 mentioned this pull request Jun 22, 2022

Update existing solvers to new interface #1177

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate PROCESS solver to new interface #1146

Migrate PROCESS solver to new interface #1146

hsaunders1904 commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jun 14, 2022 •

edited

Loading

je-cook left a comment

je-cook Jun 14, 2022

hsaunders1904 Jun 15, 2022

je-cook Jun 15, 2022

sonarcloud bot commented Jun 15, 2022

Migrate PROCESS solver to new interface #1146

Migrate PROCESS solver to new interface #1146

Conversation

hsaunders1904 commented Jun 14, 2022 • edited Loading

Linked Issues

Description

Interface Changes

Checklist

codecov-commenter commented Jun 14, 2022 • edited Loading

Codecov Report

je-cook left a comment

Choose a reason for hiding this comment

je-cook Jun 14, 2022

Choose a reason for hiding this comment

hsaunders1904 Jun 15, 2022

Choose a reason for hiding this comment

je-cook Jun 15, 2022

Choose a reason for hiding this comment

sonarcloud bot commented Jun 15, 2022

hsaunders1904 commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jun 14, 2022 •

edited

Loading