Skip to content
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

hotfix for CoilSet with NFP or stellarator symmetry #982

Merged
merged 7 commits into from
Apr 5, 2024
Merged

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Apr 3, 2024

Resolves #959.

Also preserves NFP and sym attributes when converting a CoilSet to FourierXYZ or SplineXYZ curve types.

@ddudt ddudt added bug Something isn't working coil stuff relating to coils and coil optimization EZ-review labels Apr 3, 2024
@ddudt ddudt requested a review from dpanici April 3, 2024 20:29
@ddudt ddudt changed the title hotfix for coilset conversion symmetry hotfix for CoilSet with NFP or stellarator symmetry Apr 3, 2024
@ddudt ddudt self-assigned this Apr 3, 2024
Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

MixedCoilSet don't have the NFP attribute iirc right? that is causing a test to fail, might want to make a separate conversion method for the subclass that avoids passing in NFP

Copy link
Contributor

github-actions bot commented Apr 3, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +1.04 +/- 2.94     | +1.82e-02 +/- 5.14e-02 |  1.77e+00 +/- 3.2e-02  |  1.75e+00 +/- 4.0e-02  |
 test_build_transform_fft_midres         |     +0.16 +/- 1.15     | +3.08e-03 +/- 2.23e-02 |  1.94e+00 +/- 2.1e-02  |  1.94e+00 +/- 8.1e-03  |
 test_build_transform_fft_highres        |     +0.14 +/- 0.77     | +3.36e-03 +/- 1.80e-02 |  2.33e+00 +/- 1.4e-02  |  2.33e+00 +/- 1.1e-02  |
 test_equilibrium_init_lowres            |     +1.67 +/- 2.82     | +1.57e-01 +/- 2.65e-01 |  9.55e+00 +/- 2.6e-01  |  9.40e+00 +/- 6.0e-02  |
 test_equilibrium_init_medres            |     +0.79 +/- 5.34     | +7.96e-02 +/- 5.37e-01 |  1.01e+01 +/- 5.3e-01  |  1.01e+01 +/- 5.9e-02  |
 test_equilibrium_init_highres           |     +2.44 +/- 3.78     | +2.88e-01 +/- 4.47e-01 |  1.21e+01 +/- 4.2e-01  |  1.18e+01 +/- 1.6e-01  |
 test_objective_compile_dshape_current   |     -0.06 +/- 9.96     | -1.94e-03 +/- 3.47e-01 |  3.48e+00 +/- 2.3e-01  |  3.48e+00 +/- 2.6e-01  |
 test_objective_compile_atf              |     +0.10 +/- 1.87     | +6.85e-03 +/- 1.34e-01 |  7.16e+00 +/- 1.0e-01  |  7.15e+00 +/- 8.9e-02  |
 test_objective_compute_dshape_current   |     +0.03 +/- 1.91     | +1.23e-06 +/- 7.45e-05 |  3.89e-03 +/- 4.9e-05  |  3.89e-03 +/- 5.6e-05  |
 test_objective_compute_atf              |     +5.04 +/- 2.27     | +8.91e-04 +/- 4.01e-04 |  1.86e-02 +/- 3.7e-04  |  1.77e-02 +/- 1.6e-04  |
 test_objective_jac_dshape_current       |     -0.17 +/- 6.44     | -6.99e-05 +/- 2.67e-03 |  4.14e-02 +/- 1.9e-03  |  4.15e-02 +/- 1.9e-03  |
 test_objective_jac_atf                  |     -2.22 +/- 3.31     | -4.23e-02 +/- 6.32e-02 |  1.86e+00 +/- 2.6e-02  |  1.91e+00 +/- 5.8e-02  |
 test_perturb_1                          |     +0.66 +/- 3.93     | +9.38e-02 +/- 5.54e-01 |  1.42e+01 +/- 4.1e-01  |  1.41e+01 +/- 3.8e-01  |
 test_perturb_2                          |     -0.39 +/- 2.53     | -7.35e-02 +/- 4.83e-01 |  1.90e+01 +/- 3.7e-01  |  1.91e+01 +/- 3.1e-01  |
 test_proximal_jac_atf                   |     -1.11 +/- 1.24     | -8.02e-02 +/- 8.91e-02 |  7.11e+00 +/- 7.2e-02  |  7.19e+00 +/- 5.2e-02  |
 test_proximal_freeb_compute             |     -0.59 +/- 1.03     | -7.57e-04 +/- 1.33e-03 |  1.28e-01 +/- 8.4e-04  |  1.28e-01 +/- 1.0e-03  |
 test_proximal_freeb_jac                 |     +0.19 +/- 1.92     | +1.36e-02 +/- 1.37e-01 |  7.18e+00 +/- 1.2e-01  |  7.16e+00 +/- 7.4e-02  |

desc/plotting.py Outdated Show resolved Hide resolved
@ddudt ddudt requested review from dpanici and f0uriest April 4, 2024 16:17
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 95.32%. Comparing base (f9e43c0) to head (1f6c56c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
- Coverage   95.34%   95.32%   -0.03%     
==========================================
  Files          87       87              
  Lines       21706    21717      +11     
==========================================
+ Hits        20696    20702       +6     
- Misses       1010     1015       +5     
Files Coverage Δ
desc/plotting.py 95.89% <80.00%> (-0.07%) ⬇️
desc/coils.py 97.54% <62.50%> (-0.97%) ⬇️

Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

Adding a kwarg to plot only unique coils can't hurt I think, so I am in favor of adding that

@ddudt ddudt requested a review from dpanici April 4, 2024 20:21
@ddudt
Copy link
Collaborator Author

ddudt commented Apr 4, 2024

patch coverage will be low but I think we should override that

Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

slightly uneasy with the lack of tests but I'll allow it

@ddudt
Copy link
Collaborator Author

ddudt commented Apr 5, 2024

slightly uneasy with the lack of tests but I'll allow it

This PR is just fixing existing bugs. The only code that isn't covered by tests is MixedCoilSet.to_XYZ. I could make a new test for it but I thought we were trying to minimize the number of tests

@ddudt ddudt merged commit 5e34d17 into master Apr 5, 2024
17 of 18 checks passed
@ddudt ddudt deleted the dd/coilset_sym branch April 5, 2024 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working coil stuff relating to coils and coil optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make plotting coils from a symmetric coilset plot all the coils
4 participants