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

Update equilibria in desc.examples #1406

Merged
merged 31 commits into from
Dec 11, 2024
Merged

Update equilibria in desc.examples #1406

merged 31 commits into from
Dec 11, 2024

Conversation

f0uriest
Copy link
Member

@f0uriest f0uriest commented Nov 21, 2024

Resolves #1185
Resolves #1186

Re-ran them all and manually verified they look good by eye and the usual metrics (low force error etc).

@f0uriest f0uriest marked this pull request as draft November 21, 2024 04:42
Copy link
Contributor

github-actions bot commented Nov 21, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +1.94 +/- 7.86     | +1.03e-02 +/- 4.16e-02 |  5.40e-01 +/- 3.6e-02  |  5.30e-01 +/- 2.2e-02  |
 test_equilibrium_init_medres            |     +0.45 +/- 2.10     | +1.89e-02 +/- 8.84e-02 |  4.22e+00 +/- 6.3e-02  |  4.20e+00 +/- 6.2e-02  |
 test_equilibrium_init_highres           |     -0.78 +/- 1.84     | -4.35e-02 +/- 1.03e-01 |  5.54e+00 +/- 7.2e-02  |  5.58e+00 +/- 7.4e-02  |
 test_objective_compile_dshape_current   |     -0.46 +/- 2.50     | -1.85e-02 +/- 9.98e-02 |  3.97e+00 +/- 7.0e-02  |  3.98e+00 +/- 7.2e-02  |
 test_objective_compute_dshape_current   |     -4.06 +/- 1.79     | -2.18e-04 +/- 9.60e-05 |  5.15e-03 +/- 6.7e-05  |  5.36e-03 +/- 6.9e-05  |
 test_objective_jac_dshape_current       |     +0.10 +/- 5.27     | +4.17e-05 +/- 2.29e-03 |  4.34e-02 +/- 2.0e-03  |  4.34e-02 +/- 1.1e-03  |
 test_perturb_2                          |     -1.05 +/- 2.86     | -2.10e-01 +/- 5.76e-01 |  1.99e+01 +/- 3.6e-01  |  2.01e+01 +/- 4.5e-01  |
 test_proximal_freeb_jac                 |     -0.03 +/- 1.26     | -2.21e-03 +/- 9.34e-02 |  7.44e+00 +/- 7.3e-02  |  7.44e+00 +/- 5.9e-02  |
-test_solve_fixed_iter                   |    +10.44 +/- 1.42     | +3.19e+00 +/- 4.34e-01 |  3.37e+01 +/- 3.2e-01  |  3.06e+01 +/- 2.9e-01  |
+test_LinearConstraintProjection_build   |    -55.72 +/- 1.72     | -1.35e+01 +/- 4.19e-01 |  1.08e+01 +/- 3.7e-01  |  2.43e+01 +/- 2.0e-01  |
+test_build_transform_fft_midres         |    -11.20 +/- 3.08     | -7.65e-02 +/- 2.10e-02 |  6.06e-01 +/- 1.4e-02  |  6.83e-01 +/- 1.6e-02  |
 test_build_transform_fft_highres        |     -6.84 +/- 4.62     | -7.19e-02 +/- 4.86e-02 |  9.79e-01 +/- 4.6e-02  |  1.05e+00 +/- 1.5e-02  |
 test_equilibrium_init_lowres            |    -10.86 +/- 8.74     | -4.93e-01 +/- 3.97e-01 |  4.05e+00 +/- 3.9e-01  |  4.54e+00 +/- 8.5e-02  |
 test_objective_compile_atf              |     +7.64 +/- 4.83     | +6.18e-01 +/- 3.91e-01 |  8.70e+00 +/- 2.8e-01  |  8.08e+00 +/- 2.7e-01  |
 test_objective_compute_atf              |     +0.59 +/- 3.51     | +9.30e-05 +/- 5.53e-04 |  1.58e-02 +/- 5.3e-04  |  1.57e-02 +/- 1.7e-04  |
 test_objective_jac_atf                  |     +4.34 +/- 7.65     | +8.62e-02 +/- 1.52e-01 |  2.07e+00 +/- 1.5e-01  |  1.99e+00 +/- 1.8e-02  |
 test_perturb_1                          |     +0.02 +/- 6.60     | +2.95e-03 +/- 9.71e-01 |  1.47e+01 +/- 5.1e-01  |  1.47e+01 +/- 8.3e-01  |
 test_proximal_jac_atf                   |     -0.75 +/- 1.09     | -6.25e-02 +/- 9.00e-02 |  8.23e+00 +/- 5.5e-02  |  8.30e+00 +/- 7.1e-02  |
 test_proximal_freeb_compute             |     -0.13 +/- 1.05     | -2.51e-04 +/- 2.11e-03 |  2.00e-01 +/- 1.5e-03  |  2.01e-01 +/- 1.4e-03  |
-test_solve_fixed_iter_compiled          |    +26.34 +/- 7.06     | +4.61e+00 +/- 1.24e+00 |  2.21e+01 +/- 1.1e+00  |  1.75e+01 +/- 4.7e-01  |

@dpanici
Copy link
Collaborator

dpanici commented Nov 21, 2024

This might change/need checking again once #1073 is in (or maybe base this off of #1073?)

@dpanici dpanici added skip_changelog No need to update changelog on this PR waiting for other PRs labels Nov 25, 2024
@dpanici
Copy link
Collaborator

dpanici commented Dec 4, 2024

I will approve this once merge conflict is resolved, but I also want to note that I'd like each notebook to be re-ran as well. The 3D plot-containing ones may have to be each separate PRs to avoid the issues we have with the git diffs being too large and failing our tests. I will make an issue for this

@f0uriest f0uriest changed the base branch from master to rc/stopping_criteria December 4, 2024 04:40
@dpanici
Copy link
Collaborator

dpanici commented Dec 4, 2024

Could try to reduce test time as well #914 by reducing res or not doing continuation for some of the examples

@f0uriest f0uriest changed the base branch from rc/stopping_criteria to master December 4, 2024 23:57
@f0uriest f0uriest marked this pull request as ready for review December 4, 2024 23:58
unalmis
unalmis previously approved these changes Dec 10, 2024
desc/examples/ATF Outdated Show resolved Hide resolved
@f0uriest f0uriest requested a review from YigitElma December 10, 2024 21:58
@@ -141,6 +141,7 @@ def parse_axis(axis, NFP=1, sym=True, surface=None):
axis[:, 1],
axis[:, 2],
axis[:, 0].astype(int),
axis[:, 0].astype(int),
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this doing? seems to be passing in the same modes for Rn as Zn?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems off? or do we ensure before this that we have the full mode spectrum in n and just set the non-symmetric modes to zero if it is stell symmetric?

dpanici
dpanici previously approved these changes Dec 11, 2024
YigitElma
YigitElma previously approved these changes Dec 11, 2024
Copy link
Collaborator

@YigitElma YigitElma left a comment

Choose a reason for hiding this comment

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

I think we can also reduce the radial resolution of ATF and HELITRON (also change some solver tolerances) to get the same equilibrium, as you did here for W7-X. Those are the simplest examples but have the highest radial resolution. It would help reducing the CI testing time too.

@YigitElma
Copy link
Collaborator

I found these to be also working. For ATF,

# spectral resolution
L_rad  =  8, 16
M_pol  =  12
N_tor  =  0x4, 4
M_grid =  12, 16
N_grid =  0x4, 6

For HELIOTRON,

# spectral resolution
L_rad  =  8, 18
M_pol  =  6, 12
N_tor  =  0,  0,  3
M_grid =  12, 18
N_grid =  0,  0,  6

@f0uriest f0uriest dismissed stale reviews from dpanici, unalmis, and YigitElma via 04714ed December 11, 2024 15:11
@f0uriest
Copy link
Member Author

I found these to be also working. For ATF,

# spectral resolution
L_rad  =  8, 16
M_pol  =  12
N_tor  =  0x4, 4
M_grid =  12, 16
N_grid =  0x4, 6

For HELIOTRON,

# spectral resolution
L_rad  =  8, 18
M_pol  =  6, 12
N_tor  =  0,  0,  3
M_grid =  12, 18
N_grid =  0,  0,  6

Reducing the resolution causes some tests to fail so I'm keeping the old one for now. You can make a new PR for those if you want.

@YigitElma
Copy link
Collaborator

I found these to be also working. For ATF,

# spectral resolution
L_rad  =  8, 16
M_pol  =  12
N_tor  =  0x4, 4
M_grid =  12, 16
N_grid =  0x4, 6

For HELIOTRON,

# spectral resolution
L_rad  =  8, 18
M_pol  =  6, 12
N_tor  =  0,  0,  3
M_grid =  12, 18
N_grid =  0,  0,  6

Reducing the resolution causes some tests to fail so I'm keeping the old one for now. You can make a new PR for those if you want.

Thanks for trying🙏

@f0uriest f0uriest merged commit 8804180 into master Dec 11, 2024
26 checks passed
@f0uriest f0uriest deleted the rc/examples branch December 11, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip_changelog No need to update changelog on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check + Update old notebooks on a recurring basis Some examples do not run as intended in desc/examples
4 participants