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

PEST coordinate system basis vectors #1090

Merged
merged 22 commits into from
Jul 22, 2024
Merged

PEST coordinate system basis vectors #1090

merged 22 commits into from
Jul 22, 2024

Conversation

unalmis
Copy link
Collaborator

@unalmis unalmis commented Jul 1, 2024

Generalizes theta_PEST basis vectors for toroidal stream function and changes e_phi docstring since it is somewhat misleading.

While debugging a test that only assumes nested flux surfaces, I checked the computation of the pest sfl Jacobian which looked wrong. We were computing

# e_rho fixed theta and zeta, e_vartheta fixed rho and zeta, e_phi at fixed rho and theta
dot(data["e_rho|t,z"], cross(data["e_vartheta|r,z"], data["e_phi|r,t"]))

when we should have been computing

# e_rho fixed vartheta and phi, e_vartheta at fixed rho and phi, e_phi at fixed rho and vartheta
dot(data["e_rho|v,p"], cross(data["e_vartheta|r,p"], data["e_phi|r,v"]))

Assuming $\zeta = \phi$, only the second of these vectors are the same between the two compuations; the first and third are of course not. However, the triple scalar product suppresses the differences so that the final result is equal when $\zeta = \phi$.

@unalmis unalmis changed the base branch from master to basis_bug July 1, 2024 02:55
@unalmis unalmis changed the title Generalizes theta_PEST basis vectors Generalizes PEST coordinate system basis vectors for toroidal angle Jul 1, 2024
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.23%. Comparing base (8085dc7) to head (2dad8f8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1090   +/-   ##
=======================================
  Coverage   95.23%   95.23%           
=======================================
  Files          87       87           
  Lines       21908    21918   +10     
=======================================
+ Hits        20863    20873   +10     
  Misses       1045     1045           
Files Coverage Δ
desc/compute/_basis_vectors.py 100.00% <100.00%> (ø)
desc/compute/_metric.py 100.00% <100.00%> (ø)
desc/compute/_surface.py 100.00% <ø> (ø)

Copy link
Contributor

github-actions bot commented Jul 1, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.89 +/- 6.57     | +4.49e-03 +/- 3.31e-02 |  5.08e-01 +/- 3.2e-02  |  5.04e-01 +/- 6.3e-03  |
 test_build_transform_fft_midres         |     +0.84 +/- 1.74     | +4.92e-03 +/- 1.02e-02 |  5.91e-01 +/- 7.5e-03  |  5.87e-01 +/- 6.9e-03  |
 test_build_transform_fft_highres        |     +1.24 +/- 3.87     | +1.20e-02 +/- 3.77e-02 |  9.86e-01 +/- 3.6e-02  |  9.74e-01 +/- 1.2e-02  |
 test_equilibrium_init_lowres            |     +0.93 +/- 0.64     | +3.37e-02 +/- 2.33e-02 |  3.65e+00 +/- 1.5e-02  |  3.62e+00 +/- 1.8e-02  |
 test_equilibrium_init_medres            |     +0.14 +/- 0.87     | +5.87e-03 +/- 3.55e-02 |  4.08e+00 +/- 2.0e-02  |  4.08e+00 +/- 3.0e-02  |
 test_equilibrium_init_highres           |     +0.16 +/- 0.59     | +8.91e-03 +/- 3.22e-02 |  5.47e+00 +/- 2.3e-02  |  5.46e+00 +/- 2.3e-02  |
 test_objective_compile_dshape_current   |     +0.40 +/- 0.92     | +1.52e-02 +/- 3.48e-02 |  3.78e+00 +/- 3.4e-02  |  3.76e+00 +/- 4.5e-03  |
 test_objective_compile_atf              |     +0.02 +/- 3.74     | +1.89e-03 +/- 3.04e-01 |  8.11e+00 +/- 2.1e-01  |  8.11e+00 +/- 2.2e-01  |
 test_objective_compute_dshape_current   |     +0.29 +/- 4.92     | +3.63e-06 +/- 6.23e-05 |  1.27e-03 +/- 2.4e-05  |  1.27e-03 +/- 5.7e-05  |
 test_objective_compute_atf              |     -0.97 +/- 5.77     | -4.07e-05 +/- 2.42e-04 |  4.15e-03 +/- 1.8e-04  |  4.19e-03 +/- 1.7e-04  |
 test_objective_jac_dshape_current       |     +1.18 +/- 10.20    | +4.22e-04 +/- 3.65e-03 |  3.62e-02 +/- 2.2e-03  |  3.58e-02 +/- 2.9e-03  |
 test_objective_jac_atf                  |     -1.75 +/- 3.22     | -3.25e-02 +/- 5.97e-02 |  1.83e+00 +/- 4.2e-02  |  1.86e+00 +/- 4.2e-02  |
 test_perturb_1                          |     +0.00 +/- 1.37     | +5.67e-04 +/- 1.78e-01 |  1.30e+01 +/- 1.7e-01  |  1.30e+01 +/- 3.6e-02  |
 test_perturb_2                          |     +0.04 +/- 0.89     | +7.20e-03 +/- 1.59e-01 |  1.78e+01 +/- 5.6e-02  |  1.78e+01 +/- 1.5e-01  |
 test_proximal_jac_atf                   |     +0.84 +/- 1.03     | +6.13e-02 +/- 7.48e-02 |  7.32e+00 +/- 4.6e-02  |  7.26e+00 +/- 5.9e-02  |
 test_proximal_freeb_compute             |     +1.15 +/- 0.79     | +2.00e-03 +/- 1.37e-03 |  1.77e-01 +/- 1.1e-03  |  1.75e-01 +/- 8.1e-04  |
 test_proximal_freeb_jac                 |     +1.17 +/- 1.54     | +8.40e-02 +/- 1.11e-01 |  7.28e+00 +/- 7.2e-02  |  7.19e+00 +/- 8.4e-02  |
 test_solve_fixed_iter                   |     +0.66 +/- 4.38     | +1.18e-01 +/- 7.81e-01 |  1.80e+01 +/- 5.9e-01  |  1.78e+01 +/- 5.1e-01  |

@unalmis unalmis added the low priority Nice to have, but not needed right away label Jul 1, 2024
@unalmis unalmis added the documentation Add documentation or better warnings etc. label Jul 1, 2024
@unalmis unalmis changed the title Generalizes PEST coordinate system basis vectors for toroidal angle PEST coordinate system basis vectors Jul 1, 2024
desc/compute/_basis_vectors.py Outdated Show resolved Hide resolved
tests/test_data_index.py Outdated Show resolved Hide resolved
dpanici
dpanici previously requested changes Jul 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.

remove e_phi|R,Z

@unalmis unalmis requested review from f0uriest and dpanici July 3, 2024 20:00
f0uriest
f0uriest previously approved these changes Jul 3, 2024
@unalmis unalmis changed the base branch from basis_bug to master July 7, 2024 17:22
@unalmis unalmis dismissed f0uriest’s stale review July 7, 2024 17:22

The base branch was changed.

@unalmis unalmis changed the base branch from master to basis_bug July 7, 2024 17:34
@unalmis unalmis changed the base branch from basis_bug to master July 11, 2024 20:42
@unalmis unalmis requested a review from f0uriest July 11, 2024 20:56
@unalmis
Copy link
Collaborator Author

unalmis commented Jul 11, 2024

Just fixed merge conflicts since last review

@dpanici
Copy link
Collaborator

dpanici commented Jul 17, 2024

Add notation on basis to list of variables

@@ -1407,11 +1400,13 @@ def _e_sup_zeta_zz(params, transforms, profiles, data, **kwargs):
"desc.geometry.surface.FourierRZToroidalSurface",
"desc.geometry.core.Surface",
],
aliases=["e_phi"],
# Our usual notation implies e_phi = (∂X/∂ϕ)|R,Z = R ϕ̂, but we need to alias e_phi
# to e_phi|r,t = (∂X/∂ϕ)|ρ,θ for compatibility with older versions of the code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really care about backwards compatibility here? I doubt a lot of users have code that depends on computing "e_phi"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could deprecate it later in that case

@unalmis unalmis dismissed dpanici’s stale review July 22, 2024 02:44

done in clebsch pr

@unalmis unalmis merged commit 4b21abc into master Jul 22, 2024
18 checks passed
@unalmis unalmis deleted the sfl_coordinates branch July 22, 2024 02:51
@unalmis unalmis added the theory Requires theory work before coding label Jul 22, 2024
@unalmis unalmis self-assigned this Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Add documentation or better warnings etc. low priority Nice to have, but not needed right away theory Requires theory work before coding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants