Refactor fusion reactions for dataclass - #4592
chris-ashe wants to merge 22 commits into
Conversation
…ity in calculations and tests
…y for consistency across the codebase
…aged' terminology for consistency across the codebase
…aged' terminology for consistency across the codebase
…nology for consistency across the codebase
…nology for consistency across the codebase
…ed' terminology for consistency across the codebase
…minology for consistency across the codebase
…ons classes for improved modularity and clarity in calculations
…lasmaReactions class for improved organization and clarity
chris-ashe
added
Physics
…terminology for consistency across the codebase
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4592 +/- ##
==========================================
+ Coverage 49.91% 50.09% +0.18%
==========================================
Files 151 151
Lines 29860 29958 +98
==========================================
+ Hits 14904 15008 +104
+ Misses 14956 14950 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…wer density variables and update related calculations for consistency
…nology for consistency across the codebase
…action rate densities
…minology for consistency across plasma reactions and related calculations
…nd update beam reaction calculations for current drive methods
timothy-nunn
left a comment
timothy-nunn
left a comment
There was a problem hiding this comment.
Will need a modeller to check some of the model changes (even if results have not changed) and new tests
Regression test failing:
FAILED tests/regression/test_process_input_files.py::test_input_file[stellarator_helias] - AttributeError: module 'process.models.physics.fusion_reactions' has no attribute 'FusionReactionRate'
Some unit tests are also failing
| def _make_beam_fusion_reactions(): | ||
| return reactions.BeamReactions(data=DataStructure()) |
There was a problem hiding this comment.
This should be a fixture rather than a helper function
| fusden_plasma_alpha_vol_avg: float = 0.0 | ||
| fusden_plasma_protons_vol_avg: float = 0.0 | ||
|
|
||
| def __iadd__(self, other: "FusionYieldDensities") -> Self: |
There was a problem hiding this comment.
I would add some validation to this method which raises an error if other is not an instance of type(self)
| fusden_plasma_alpha_vol_avg: float = 0.0 | ||
| fusden_plasma_protons_vol_avg: float = 0.0 | ||
|
|
||
| def __iadd__(self, other: "FusionYieldDensities") -> Self: |
There was a problem hiding this comment.
Can we also add a specific unit test just dedicated to testing the addition of two FusionYieldDensities works correctly
| fusden_plasma_alpha_vol_avg: float = 0.0 | ||
| fusden_plasma_protons_vol_avg: float = 0.0 | ||
|
|
||
| def __iadd__(self, other: "FusionYieldDensities") -> Self: |
There was a problem hiding this comment.
| def __iadd__(self, other: "FusionYieldDensities") -> Self: | |
| def __iadd__(self, other: FusionYieldDensities) -> Self: |
Add this line to the top of the file also
from __future__ import annotations| return self | ||
|
|
||
|
|
||
| class PlasmaReactions: |
There was a problem hiding this comment.
If this is a model in main.py it should inherit from process.core.model.Model like other models.
Take a look at another model but this means it should not explicitly be passed the data structure.
| po.oblnkl(self.outfile) | ||
|
|
||
|
|
||
| class BeamReactions: |
There was a problem hiding this comment.
Same comment here
https://github.com/ukaea/PROCESS/pull/4592/changes#r4045131410
There was a problem hiding this comment.
We need to create an issue to remove the state in these models. Any self.xyz data should be in the data structure!
If this is purely a refactor there shouldn't be any model changes. Can you clarify? |
4 participants
Checklist
I confirm that I have completed the following checks: