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

Update power cycle solver for new api #1167

Conversation

hsaunders1904
Copy link
Contributor

Linked Issues

Linked to #956

Description

Minor updates to EUDEMO power cycle solver so that it works with the new solver 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

Copy link
Contributor

@je-cook je-cook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was surprisingly easy! (if the tests like it)

@je-cook
Copy link
Contributor

je-cook commented Jun 20, 2022

Looks like there is some failure that we missed in the process update because of some docs hacking I did...

@hsaunders1904
Copy link
Contributor Author

This is my fault. I've just rebased the target branch (so the power cycle solver actually exists) - kept the process.run file where it should have been deleted. Fix incoming

A bad rebase restored a previously deleted file. Re-delete it
@hsaunders1904 hsaunders1904 requested a review from a team as a code owner June 20, 2022 10:16
@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@je-cook je-cook merged commit 1733097 into hsaunders1904/956_move_existing_solvers_to_new_interface Jun 20, 2022
@je-cook je-cook deleted the hsaunders1904/956_update_power_cycle_solver_for_new_api branch June 20, 2022 13:22
je-cook added a commit that referenced this pull request Jun 22, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants