-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix io of MixedCoilSets #1016
Fix io of MixedCoilSets #1016
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1016 +/- ##
==========================================
+ Coverage 94.94% 94.95% +0.01%
==========================================
Files 87 87
Lines 21699 21704 +5
==========================================
+ Hits 20602 20609 +7
+ Misses 1097 1095 -2
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | -6.00 +/- 3.65 | -3.30e-02 +/- 2.01e-02 | 5.18e-01 +/- 1.7e-02 | 5.51e-01 +/- 1.1e-02 |
test_build_transform_fft_midres | -5.51 +/- 3.42 | -3.45e-02 +/- 2.15e-02 | 5.92e-01 +/- 2.1e-02 | 6.27e-01 +/- 5.4e-03 |
test_build_transform_fft_highres | -2.35 +/- 1.60 | -2.41e-02 +/- 1.64e-02 | 1.00e+00 +/- 1.1e-02 | 1.03e+00 +/- 1.2e-02 |
test_equilibrium_init_lowres | -6.66 +/- 4.03 | -2.84e-01 +/- 1.71e-01 | 3.98e+00 +/- 1.4e-01 | 4.26e+00 +/- 1.0e-01 |
test_equilibrium_init_medres | -5.96 +/- 6.41 | -2.76e-01 +/- 2.97e-01 | 4.36e+00 +/- 7.9e-02 | 4.64e+00 +/- 2.9e-01 |
test_equilibrium_init_highres | -1.82 +/- 2.60 | -1.14e-01 +/- 1.62e-01 | 6.14e+00 +/- 8.3e-02 | 6.25e+00 +/- 1.4e-01 |
test_objective_compile_dshape_current | -1.14 +/- 1.51 | -4.15e-02 +/- 5.49e-02 | 3.59e+00 +/- 1.8e-02 | 3.63e+00 +/- 5.2e-02 |
test_objective_compile_atf | -1.56 +/- 1.13 | -1.10e-01 +/- 8.04e-02 | 6.99e+00 +/- 1.7e-02 | 7.10e+00 +/- 7.9e-02 |
test_objective_compute_dshape_current | -1.85 +/- 3.22 | -7.87e-05 +/- 1.37e-04 | 4.18e-03 +/- 7.3e-05 | 4.26e-03 +/- 1.2e-04 |
test_objective_compute_atf | +2.45 +/- 2.82 | +4.38e-04 +/- 5.05e-04 | 1.83e-02 +/- 3.1e-04 | 1.79e-02 +/- 4.0e-04 |
test_objective_jac_dshape_current | -1.12 +/- 6.21 | -4.60e-04 +/- 2.54e-03 | 4.04e-02 +/- 2.1e-03 | 4.09e-02 +/- 1.4e-03 |
test_objective_jac_atf | -2.01 +/- 3.70 | -3.77e-02 +/- 6.95e-02 | 1.84e+00 +/- 3.8e-02 | 1.88e+00 +/- 5.8e-02 |
test_perturb_1 | -3.11 +/- 3.42 | -4.21e-01 +/- 4.62e-01 | 1.31e+01 +/- 3.4e-01 | 1.35e+01 +/- 3.1e-01 |
test_perturb_2 | -3.16 +/- 2.63 | -5.87e-01 +/- 4.89e-01 | 1.80e+01 +/- 3.7e-01 | 1.86e+01 +/- 3.2e-01 |
test_proximal_jac_atf | -1.32 +/- 1.07 | -9.62e-02 +/- 7.76e-02 | 7.17e+00 +/- 4.4e-02 | 7.26e+00 +/- 6.4e-02 |
test_proximal_freeb_compute | -0.19 +/- 0.79 | -3.65e-04 +/- 1.55e-03 | 1.97e-01 +/- 1.1e-03 | 1.97e-01 +/- 1.1e-03 |
test_proximal_freeb_jac | +1.52 +/- 1.99 | +1.13e-01 +/- 1.48e-01 | 7.51e+00 +/- 1.2e-01 | 7.40e+00 +/- 8.4e-02 | |
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.
Test?
I can confirm this works, although it still throws the warning: As a test, maybe we move the |
The commit I pushed also gets rid of the runtime warning, I think this is fine as the coiilset |
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.
I moved the "Dummy" coil sets to tests/conftest.py
so we can reference them in other tests in future PRs.
@ddudt it is not a real test if it is in conftest, as then we never check if the coilset when loaded in is equivalent to the original coilset. Can you revert your commit? or re-add the test? |
I think it still gets saved in conftest, and the original error was that it wasn't able to save the coilset at all, not that it was saved and loaded incorrectly. I have tests ready in other PRs that use this coilset and will effectively cover what you want. |
- Adds the objective `CoilsetMinDistance`, which returns the minimum distance to another coil for each coil in a coilset. Resolves #898 - Adds the objective `PlasmaCoilsetMinDistance`, which returns the minimum distance to the plasma surface for each coil in a coilset. Resolves #900, #947 Dependencies: - #1016 - #1017 - #1018 TODO: - [x] unit tests for `CoilsetMinDistance` - [x] unit tests for `PlasmaCoilsetMinDistance` - [x] regression test using both objectives in a coil optimization - [x] make it work with surface and/or equilibrium to resolve #947
Resolves #1013