-
Notifications
You must be signed in to change notification settings - Fork 12
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
Major refactoring of the parametric shapes and parametric components default parameters #354
Major refactoring of the parametric shapes and parametric components default parameters #354
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #354 +/- ##
===========================================
+ Coverage 95.78% 96.06% +0.27%
===========================================
Files 49 49
Lines 2871 2820 -51
===========================================
- Hits 2750 2709 -41
+ Misses 121 111 -10
Continue to review full report at Codecov.
|
This is an epic improvement |
@shimwell I estimated an approximately 27% code reduction if this was made to all parametric components ! A similar thing could be done to the reactor classes since they are some default parameters repetitions |
Thanks for this Remi, I'm going to take a look tomorrow. A bit of progress elsewhere today as I merged the new cutters and simple neutronics simulations are working |
This looks great, but I also wondering if we could do with a few more tests before implementing this. Many of the parametric shape tests just check the solid is not None and something has been constructed. Adding to my ToDo list |
@shimwell we could surely do more testing yes. What kind of tests are you thinking of ? |
I'm thinking some more tests that check the arguments, including kwargs impact the shape construction. |
Like sanity tests that check the arguments we set in kwargs are kept after construction ? |
Closing this. Extended in #357 |
Proposed changes
@billingsley-john proposed in #115 a way of simplifying classes initialisation with a default dict.
However, since most of these parameters are already defaulted at the Shape level (which is inherited by all classes in paramak), this
default_dict
can be replaced by thekwargs
.This PR shows how to perform this refactoring and includes it for all the parametric shapes (eg,
RotateStraightShape
,ExtrudeSplineShape
, etc.) and for the shapes inherited fromPlasma
(PlasmaFromPoints
,PlasmaBoundaries
).In the future, this should be done for all components which would:
Types of changes
What types of changes does your code introduce to the Paramak?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
@billingsley-john @shimwell what are your thoughts ?
this could be a nice "Good first issue".