-
Notifications
You must be signed in to change notification settings - Fork 16
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
Adding toroidal harmonic notebooks #3681
Adding toroidal harmonic notebooks #3681
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/toroidal-harmonics #3681 +/- ##
==============================================================
- Coverage 76.62% 76.25% -0.38%
==============================================================
Files 230 231 +1
Lines 27025 27053 +28
==============================================================
- Hits 20709 20630 -79
- Misses 6316 6423 +107 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my naive understand looks really good. Could have have some tests for the function going into the main package please?
examples/equilibria/toroidal_harmonics_component_function_verification.ex.py
Show resolved
Hide resolved
I was just running our tests on develop and we get a warning from one of the conversion functions (from arccos of a number outside the range bluemira/bluemira/utilities/tools.py Line 867 in fcce20f
to np.clip((d_1**2 + d_2**2 - 4 * R_0**2) / (2 * d_1 * d_2), -1, 1) this seem to remove the warning in my quick test. I think its just floating point problems. |
27871cf
to
e123fbd
Compare
- added tests for legendre functions - improved explanations
e123fbd
to
582c036
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code perspective looks good, thanks for adding the tests. A few little things otherwise from my end good to go.
I can't comment on the actual maths maybe @CoronelBuendia or @geograham can comment more?
bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go
bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py
Outdated
Show resolved
Hide resolved
bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py
Outdated
Show resolved
Hide resolved
…approx_functions.py Co-authored-by: geograham <[email protected]>
…approx_functions.py Co-authored-by: geograham <[email protected]>
|
489a202
into
Fusion-Power-Plant-Framework:feature/toroidal-harmonics
* add toroidal harmonics approx functions and examples * working on pr comments * changes based on pr review comments - added tests for legendre functions - improved explanations * adding image referenced in singe_wire notebook * changes based on review comments * Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py Co-authored-by: geograham <[email protected]> * Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py Co-authored-by: geograham <[email protected]> --------- Co-authored-by: geograham <[email protected]>
* add toroidal harmonics approx functions and examples * working on pr comments * changes based on pr review comments - added tests for legendre functions - improved explanations * adding image referenced in singe_wire notebook * changes based on review comments * Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py Co-authored-by: geograham <[email protected]> * Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py Co-authored-by: geograham <[email protected]> --------- Co-authored-by: geograham <[email protected]>
Linked Issues
(This merge request is updated version of #3645)
Part of #3340
Description
Interface Changes
Checklist
I confirm that I have completed the following checks:
pytest tests --reactor
pre-commit run --from-ref develop --to-ref HEAD
sphinx-build -W documentation/source documentation/build