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

Stop modding of non-periodic angles #1254

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Stop modding of non-periodic angles #1254

merged 4 commits into from
Sep 19, 2024

Conversation

unalmis
Copy link
Collaborator

@unalmis unalmis commented Sep 18, 2024

Resolves #1180 and resolves #1173 .

Here is what $\theta$ looks like now on #1119 :
theta

Likewise, all the other quantities have become smooth bumps, as desired.

@unalmis unalmis self-assigned this Sep 18, 2024
Copy link
Contributor

github-actions bot commented Sep 18, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -3.13 +/- 7.15     | -1.71e-02 +/- 3.90e-02 |  5.28e-01 +/- 3.3e-02  |  5.45e-01 +/- 2.1e-02  |
 test_equilibrium_init_medres            |     -1.40 +/- 2.64     | -5.92e-02 +/- 1.11e-01 |  4.16e+00 +/- 6.1e-02  |  4.22e+00 +/- 9.3e-02  |
 test_equilibrium_init_highres           |     -0.43 +/- 2.38     | -2.39e-02 +/- 1.32e-01 |  5.52e+00 +/- 7.9e-02  |  5.54e+00 +/- 1.1e-01  |
 test_objective_compile_dshape_current   |     -0.92 +/- 1.60     | -3.65e-02 +/- 6.36e-02 |  3.93e+00 +/- 5.4e-02  |  3.97e+00 +/- 3.4e-02  |
 test_objective_compute_dshape_current   |     +4.60 +/- 53.00    | +1.67e-04 +/- 1.93e-03 |  3.81e-03 +/- 1.9e-03  |  3.64e-03 +/- 3.3e-05  |
 test_objective_jac_dshape_current       |     -5.04 +/- 5.40     | -2.11e-03 +/- 2.26e-03 |  3.97e-02 +/- 1.4e-03  |  4.18e-02 +/- 1.8e-03  |
 test_perturb_2                          |     -1.12 +/- 3.03     | -2.00e-01 +/- 5.41e-01 |  1.76e+01 +/- 2.7e-01  |  1.78e+01 +/- 4.7e-01  |
 test_proximal_freeb_jac                 |     +0.33 +/- 4.39     | +2.49e-02 +/- 3.29e-01 |  7.52e+00 +/- 5.6e-02  |  7.50e+00 +/- 3.2e-01  |
 test_solve_fixed_iter                   |     -0.24 +/- 58.96    | -1.20e-02 +/- 2.97e+00 |  5.02e+00 +/- 2.1e+00  |  5.03e+00 +/- 2.1e+00  |
 test_build_transform_fft_midres         |     -0.21 +/- 6.68     | -1.29e-03 +/- 4.11e-02 |  6.14e-01 +/- 3.9e-02  |  6.16e-01 +/- 1.3e-02  |
 test_build_transform_fft_highres        |     -0.20 +/- 3.22     | -2.00e-03 +/- 3.24e-02 |  1.01e+00 +/- 3.1e-02  |  1.01e+00 +/- 1.1e-02  |
 test_equilibrium_init_lowres            |     -0.79 +/- 1.55     | -3.04e-02 +/- 5.97e-02 |  3.82e+00 +/- 4.2e-02  |  3.85e+00 +/- 4.3e-02  |
 test_objective_compile_atf              |     +0.35 +/- 1.17     | +2.79e-02 +/- 9.36e-02 |  8.01e+00 +/- 6.3e-02  |  7.98e+00 +/- 6.9e-02  |
 test_objective_compute_atf              |     -0.93 +/- 2.07     | -9.81e-05 +/- 2.18e-04 |  1.04e-02 +/- 1.9e-04  |  1.05e-02 +/- 1.0e-04  |
 test_objective_jac_atf                  |     -0.63 +/- 3.29     | -1.23e-02 +/- 6.43e-02 |  1.94e+00 +/- 3.5e-02  |  1.96e+00 +/- 5.4e-02  |
 test_perturb_1                          |     +0.19 +/- 2.04     | +2.39e-02 +/- 2.53e-01 |  1.25e+01 +/- 1.7e-01  |  1.24e+01 +/- 1.8e-01  |
 test_proximal_jac_atf                   |     -0.17 +/- 1.02     | -1.41e-02 +/- 8.34e-02 |  8.16e+00 +/- 5.6e-02  |  8.18e+00 +/- 6.2e-02  |
 test_proximal_freeb_compute             |     +0.15 +/- 1.34     | +2.77e-04 +/- 2.48e-03 |  1.85e-01 +/- 1.3e-03  |  1.85e-01 +/- 2.1e-03  |

@unalmis unalmis marked this pull request as ready for review September 18, 2024 23:51
@unalmis unalmis requested review from a team, dpanici, kianorr, sinaatalay and YigitElma and removed request for a team September 19, 2024 00:10
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.45%. Comparing base (54becdb) to head (97d1747).
Report is 2175 commits behind head on master.

Files with missing lines Patch % Lines
desc/equilibrium/coords.py 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
- Coverage   95.45%   95.45%   -0.01%     
==========================================
  Files          95       95              
  Lines       23429    23423       -6     
==========================================
- Hits        22364    22358       -6     
  Misses       1065     1065              
Files with missing lines Coverage Δ
desc/compute/_core.py 100.00% <100.00%> (ø)
desc/equilibrium/equilibrium.py 95.71% <ø> (ø)
desc/grid.py 93.07% <ø> (ø)
desc/vmec.py 91.91% <100.00%> (+<0.01%) ⬆️
desc/equilibrium/coords.py 88.42% <95.45%> (-0.33%) ⬇️

... and 2 files with indirect coverage changes

@rahulgaur104
Copy link
Collaborator

Here is what θ looks like now on #1119

Does it work for negative zeta values? What if I give zeta in [-4pi, 4pi] to map_coordinates. Will the theta still be smooth?

@unalmis
Copy link
Collaborator Author

unalmis commented Sep 19, 2024

Here is what θ looks like now on #1119

Does it work for negative zeta values? What if I give zeta in [-4pi, 4pi] to map_coordinates. Will the theta still be smooth?

Where we start $\zeta$ seems to not matter. However there is still an issue, I believe completely separate to this PR. I have found that $\theta$ is still discontinuous every two toroidal transits, which is causing issues: #1119 (comment).

Here is a plot of $\theta(\alpha=0, \zeta)$.

tt

maybe something with $\lambda$?

@unalmis unalmis mentioned this pull request Sep 19, 2024
Copy link
Collaborator

@rahulgaur104 rahulgaur104 left a comment

Choose a reason for hiding this comment

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

Approving, but I am slightly concerned that it might break the ballooning PR.
Merge at will.

@unalmis unalmis merged commit 75eafcc into master Sep 19, 2024
24 checks passed
@unalmis unalmis deleted the ku/stop_angle_mod branch September 19, 2024 04:10
@unalmis
Copy link
Collaborator Author

unalmis commented Sep 19, 2024

For the equilibrium I computed this plot on, two toroidal transits corresponds to one poloidal transit. So it seems the information of which poloidal transit $\theta$ is at is lost somewhere

@unalmis
Copy link
Collaborator Author

unalmis commented Sep 19, 2024

So the issue is probably that I'm paramterizing my FourierChebyshev series for $\theta$ using $\alpha \in [-\pi, \pi]$ and I should be using $\alpha \in [0, 2 \pi]$. I'll look into this on the weekend.

@unalmis
Copy link
Collaborator Author

unalmis commented Sep 19, 2024

I resolved the remaining discontinuous issue fyi.

@unalmis unalmis added bug fix Something was fixed and removed bug fix Something was fixed labels Sep 23, 2024
@unalmis unalmis added the bug fix Something was fixed label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Something was fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

don't mod angles; they are not periodic Mod SFL theta by 2pi inside of vmec.VMECIO.compute_theta_coords
3 participants