Skip to content

Toroidal harmonics single coil and full coilset #3789

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Toroidal harmonics single coil and full coilset #3789

wants to merge 8 commits into from

Conversation

clmould
Copy link
Contributor

@clmould clmould commented Jan 29, 2025 •
edited by je-cook
Loading

Linked Issues

Closes #3743
Closes #3781
Closes #3340

Description

  • Added function to construct the harmonic amplitude matrix using toroidal harmonics
  • Added function to approximate psi using the harmonic amplitude matrix matrix
  • Notebook to demonstrate toroidal harmonics component function walkthrough and single coil example
  • Notebook to demonstrate use of toroidal harmonics functions on a coilset with comparison to bluemira

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 pre-commit run --from-ref develop --to-ref HEAD
  • Documentation built locally and checked sphinx-build -W documentation/source documentation/build

clmould and others added 6 commits December 16, 2024 11:14
@clmould @geograham
Adding toroidal harmonic notebooks (Fusion-Power-Plant-Framework#3681)
958b5e3 
* 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 <g.a.graham09@gmail.com>

* Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py

Co-authored-by: geograham <g.a.graham09@gmail.com>

---------

Co-authored-by: geograham <g.a.graham09@gmail.com>
@clmould
working on pr comments
4f6b5e8
@clmould
changes based on pr review comments
37ae453 
- added tests for legendre functions
- improved explanations
@clmould
changes based on review comments
d2c7f1b
@clmould @geograham
Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_…
74c060a 
…approx_functions.py

Co-authored-by: geograham <g.a.graham09@gmail.com>
@clmould @geograham
Update bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_…
d51a015 
…approx_functions.py

Co-authored-by: geograham <g.a.graham09@gmail.com>
@codecov Codecov
Copy link

codecov bot commented Jan 29, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 43.90244% with 46 lines in your changes missing coverage. Please review.

Project coverage is 76.30%. Comparing base (2d624bd) to head (599d9ed).
Report is 3 commits behind head on feature/toroidal-harmonics.

Files with missing lines Patch % Lines
...n/harmonics/toroidal_harmonics_approx_functions.py 43.20% 46 Missing ⚠️

❌ Your patch check has failed because the patch coverage (43.90%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@                      Coverage Diff                       @@
##           feature/toroidal-harmonics    #3789      +/-   ##
==============================================================
- Coverage                       76.75%   76.30%   -0.45%     
==============================================================
  Files                             230      231       +1     
  Lines                           27238    27319      +81     
==============================================================
- Hits                            20906    20847      -59     
- Misses                           6332     6472     +140

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was linked to issues Jan 30, 2025
Apply toroidal harmonics functions to Bluemira coilset #3781
Open
Investigate toroidal equivalent of functions used to create the 'harmonic_amplitude_marix' in BM spherical harmonics. #3340
Open
Apply toroidal harmonics functions to single Bluemira coil #3743
Open
@clmould clmould marked this pull request as ready for review January 30, 2025 13:06
@clmould clmould requested a review from a team as a code owner January 30, 2025 13:06
@clmould clmould force-pushed the clair/toroidal-approx-nb-single-wire-nb branch from 811dc12 to 599d9ed Compare January 30, 2025 13:11
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@je-cook
Copy link
Contributor

je-cook commented Jan 31, 2025 •
edited
Loading

fyi the docs failure is because some examples have been removed/renamed/added and need to be changed in documenation/examples.rst.
The linter failure on TODO's as @clmould and I discussed is fine for the feature branch

@je-cook je-cook force-pushed the feature/toroidal-harmonics branch from 958b5e3 to af6e058 Compare February 4, 2025 11:52
@je-cook je-cook requested review from a team as code owners February 4, 2025 11:52
@je-cook je-cook added enhancement New feature or request equilibria Tasks relating to the equilibria module labels Feb 11, 2025
geograham
geograham requested changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@geograham geograham left a comment

Choose a reason for hiding this comment

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

Hey, some tests and some comments to add. Otherwise, happy with the level of testing we have done in person.

bluemira/equilibria/optimisation/harmonics/toroidal_harmonics_approx_functions.py
return legQ


def coil_toroidal_harmonic_amplitude_matrix(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have some tests that make sure that the coil_toroidal_harmonic_amplitude_matrix and toroidal_harmonic_approximate_psi return the correct shape of matrix given eg. different numbers of control coils or max degrees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could also have a couple of regression tests for coil_toroidal_harmonic_amplitude_matrix and toroidal_harmonic_approximate_psi.

@clmould
pr review comments, sentence highlighting zero difference in the core…
9b03ba8 
…, add sentence explaining fit metric, note about slightly higher fit metric tolerance
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Apr 7, 2025

Quality Gate Passed Quality Gate passed

Issues
20 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@je-cook je-cook force-pushed the feature/toroidal-harmonics branch from af6e058 to f3df572 Compare April 7, 2025 09:12
@clmould
Copy link
Contributor Author

clmould commented Apr 7, 2025

Closing this PR as there have been a lot of changes on a newer branch which implements toroidal harmonic constraints. The comments left here are addressed in the new PR #3867.

@clmould clmould closed this Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geograham geograham geograham requested changes

Assignees
No one assigned
Labels
enhancement New feature or request equilibria Tasks relating to the equilibria module
Projects
None yet
Milestone
No milestone
3 participants
@clmould @je-cook @geograham