Papercut: enumify text based choices #3021
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
February 15, 2024 13:48
|
@oliverfunk Could you please review the papercut PR? |
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #3021 +/- ##
===========================================
- Coverage 79.79% 79.43% -0.37%
===========================================
Files 226 226
Lines 25120 25273 +153
===========================================
+ Hits 20045 20076 +31
- Misses 5075 5197 +122 ☔ View full report in Codecov by Sentry. |
oliverfunk
requested changes
Feb 19, 2024
Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com>
oliverfunk
requested changes
Feb 19, 2024
|
oliverfunk
approved these changes
Feb 20, 2024
oliverfunk
merged commit 519f50f
into
Fusion-Power-Plant-Framework:develop
3 of 4 checks passed
OceanNuclear
pushed a commit
that referenced
this pull request
Feb 20, 2024
* ✅ Enumification of text based choices in bluemira.fuel_cycle * ✅ enumified text choices in bluemira.geometry * fied pf_coil.py * ✅ enumify text based choices in bluemira.equilibria * ✅ enumify text based choices in bluemira.structural * ✅ enumify some text based choics in eudemo.eudemo * 🐛 fix circular import * 🐛 Fix Failing test_coordinates.py * 🐛 Fix failing test_tools.py * 🐛 fix failing test_pyclipper_offset.py * 🐛 remove plane=None test from test_KeyError_if_invalid_plane * 🚨 Fix test function name * Update bluemira/builders/pf_coil.py Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com> * ✅ Modify codes to resolve reviewer's comments * ✅ remove unnecessary .upper(), and a few name changes * 🐛 fix failing test_model.py * 🐛 further cleanup * 🐛 fix circular import error and duplication of CoilType * updated docstrings --------- Co-authored-by: Athoy Nilima <athoy.nilima@ukaea.uk> Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com> Co-authored-by: Oliver Funk <oli.funk@gmail.com>
je-cook
reviewed
Feb 27, 2024
| except KeyError: | ||
| raise ValueError( | ||
| "Please select a numerical stabilisation strategy" | ||
| ' from: 1) "virtual" \n 2) "feedback" 3) None.' |
There was a problem hiding this comment.
please autofill the options with the cls._member_names_ attribute eg
f"{*cls._member_names_,}"also the error message sounds like the user can input something (which they can't). Maybe something like:
f"{cls.__name__} has no strategy {value} please select from {*cls._member_names_,}"
| FEEDBACK = auto() | ||
|
|
||
| @classmethod | ||
| def _missing_(cls, value): |
There was a problem hiding this comment.
typing of value is missing def _missing_(cls, value: str):
| self.controller = VirtualController(self, gz=2.2) | ||
| elif vcontrol_type is VerticalPositionControlType.FEEDBACK: | ||
| raise NotImplementedError | ||
| else: |
There was a problem hiding this comment.
this error message will never be hit as it will error on L1073 on the initialisation of the enum, please remove the else
| try: | ||
| return cls[value.upper()] | ||
| except KeyError: | ||
| raise ValueError(f"{value} is a wrong ReactorType. Choose from: ST or Normal") from None |
There was a problem hiding this comment.
same as previous enum comments, no typing on value and error message:
f"{cls.__name__} has no entry {value} please select from {*cls._member_names_,}"
| return cls[value.upper()] | ||
| except KeyError: | ||
| raise StructuralError( | ||
| f"Unknown SubLoad type {value}. Choose from: force, moment or all" |
| from bluemira.structural.error import StructuralError | ||
| from bluemira.structural.geometry import Geometry | ||
| from bluemira.structural.loads import LoadCase | ||
| from bluemira.structural.result import Result | ||
| from bluemira.structural.symmetry import CyclicSymmetry | ||
|
|
||
|
|
||
| class BoundaryConditionMethodType(Enum): |
There was a problem hiding this comment.
rename BoundaryConditionMethod
| @@ -524,8 +547,8 @@ def _apply_boundary_conditions( | |||
| k = np.delete(k, i, axis=0) | |||
| k = np.delete(k, i, axis=1) | |||
| p = np.delete(p, i) | |||
| else: | |||
| raise StructuralError(f"Unrecognised method: {method}.") | |||
| # else: | |||
| blanket_pump_strat = H2OPumping( | ||
| 0.005, | ||
| eta_isentropic=params.bb_pump_eta_isen.value, | ||
| eta_electric=params.bb_pump_eta_el.value, | ||
| ) | ||
| bop_cycle = PredeterminedEfficiency(0.33) | ||
| else: | ||
| raise ValueError(f"Unrecognised blanket type {params.blanket_type.value}") | ||
| # else: |
| @@ -456,12 +456,13 @@ def test_hull_around_two_circles_xy_plane(self): | |||
| bounding_box = hull.bounding_box | |||
| assert bounding_box.z_min == bounding_box.z_max == 0 | |||
|
|
|||
| @pytest.mark.parametrize("bad_plane", ["ab", "", None, ["x", "y"]]) | |||
| @pytest.mark.parametrize("bad_plane", ["ab", "", ["x", "y"]]) | |||
There was a problem hiding this comment.
this test case shouldnt be removed
There was a problem hiding this comment.
OceanNuclear
pushed a commit
that referenced
this pull request
Jun 17, 2024
* ✅ Enumification of text based choices in bluemira.fuel_cycle * ✅ enumified text choices in bluemira.geometry * fied pf_coil.py * ✅ enumify text based choices in bluemira.equilibria * ✅ enumify text based choices in bluemira.structural * ✅ enumify some text based choics in eudemo.eudemo * 🐛 fix circular import * 🐛 Fix Failing test_coordinates.py * 🐛 Fix failing test_tools.py * 🐛 fix failing test_pyclipper_offset.py * 🐛 remove plane=None test from test_KeyError_if_invalid_plane * 🚨 Fix test function name * Update bluemira/builders/pf_coil.py Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com> * ✅ Modify codes to resolve reviewer's comments * ✅ remove unnecessary .upper(), and a few name changes * 🐛 fix failing test_model.py * 🐛 further cleanup * 🐛 fix circular import error and duplication of CoilType * updated docstrings --------- Co-authored-by: Athoy Nilima <athoy.nilima@ukaea.uk> Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com> Co-authored-by: Oliver Funk <oli.funk@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Linked Issues
Text based choices should be verified for spelling etc, enums will provide that functionality or possibly class attributes
Closes #{1044}
Description
Emunified a number of text based choices in several modules.
Interface Changes
Checklist
I confirm that I have completed the following checks:
pytest tests --reactorpre-commit run --from-ref develop --to-ref HEADsphinx-build -W documentation/source documentation/build