Move basis change to main compute function #1027

YigitElma · 2024-05-20T22:23:57Z

Resolves #992
Resolves #1088

All compute quantities are now returned in toroidal $(R,\phi,Z)$ coordinates.

TODO:

Implement _curve computes too
Add test_compute_everthing_xyz

…d code

github-actions · 2024-05-20T23:43:31Z

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.15 +/- 7.20     | +7.85e-04 +/- 3.70e-02 |  5.15e-01 +/- 3.6e-02  |  5.14e-01 +/- 8.9e-03  |
 test_build_transform_fft_midres         |     -0.75 +/- 4.36     | -4.54e-03 +/- 2.63e-02 |  5.98e-01 +/- 1.9e-02  |  6.02e-01 +/- 1.8e-02  |
 test_build_transform_fft_highres        |     -1.39 +/- 5.44     | -1.45e-02 +/- 5.66e-02 |  1.02e+00 +/- 3.9e-02  |  1.04e+00 +/- 4.1e-02  |
 test_equilibrium_init_lowres            |     +0.57 +/- 4.00     | +2.12e-02 +/- 1.50e-01 |  3.76e+00 +/- 1.4e-01  |  3.74e+00 +/- 4.1e-02  |
 test_equilibrium_init_medres            |     +1.76 +/- 2.35     | +7.36e-02 +/- 9.81e-02 |  4.25e+00 +/- 8.6e-02  |  4.17e+00 +/- 4.8e-02  |
 test_equilibrium_init_highres           |     +0.45 +/- 1.83     | +2.55e-02 +/- 1.03e-01 |  5.69e+00 +/- 8.1e-02  |  5.66e+00 +/- 6.5e-02  |
 test_objective_compile_dshape_current   |     -0.47 +/- 1.35     | -1.81e-02 +/- 5.17e-02 |  3.80e+00 +/- 4.3e-02  |  3.82e+00 +/- 2.9e-02  |
 test_objective_compile_atf              |     +0.58 +/- 1.94     | +4.78e-02 +/- 1.60e-01 |  8.26e+00 +/- 9.6e-02  |  8.22e+00 +/- 1.3e-01  |
 test_objective_compute_dshape_current   |     +0.45 +/- 3.33     | +5.58e-06 +/- 4.17e-05 |  1.26e-03 +/- 2.6e-05  |  1.25e-03 +/- 3.3e-05  |
 test_objective_compute_atf              |     -0.33 +/- 4.46     | -1.42e-05 +/- 1.89e-04 |  4.23e-03 +/- 1.3e-04  |  4.25e-03 +/- 1.4e-04  |
 test_objective_jac_dshape_current       |     +6.26 +/- 11.12    | +2.28e-03 +/- 4.06e-03 |  3.88e-02 +/- 3.7e-03  |  3.65e-02 +/- 1.8e-03  |
 test_objective_jac_atf                  |     -0.86 +/- 2.85     | -1.62e-02 +/- 5.36e-02 |  1.86e+00 +/- 3.0e-02  |  1.88e+00 +/- 4.4e-02  |
 test_perturb_1                          |     -0.13 +/- 1.53     | -1.74e-02 +/- 2.01e-01 |  1.31e+01 +/- 1.6e-01  |  1.32e+01 +/- 1.3e-01  |
 test_perturb_2                          |     -0.79 +/- 1.32     | -1.45e-01 +/- 2.42e-01 |  1.82e+01 +/- 2.1e-01  |  1.84e+01 +/- 1.2e-01  |
 test_proximal_jac_atf                   |     +0.12 +/- 1.08     | +8.69e-03 +/- 7.94e-02 |  7.35e+00 +/- 7.2e-02  |  7.34e+00 +/- 3.4e-02  |
 test_proximal_freeb_compute             |     -1.03 +/- 1.01     | -1.85e-03 +/- 1.81e-03 |  1.77e-01 +/- 1.4e-03  |  1.79e-01 +/- 1.1e-03  |
 test_proximal_freeb_jac                 |     -0.74 +/- 1.29     | -5.45e-02 +/- 9.55e-02 |  7.35e+00 +/- 5.3e-02  |  7.41e+00 +/- 7.9e-02  |
 test_solve_fixed_iter                   |     +1.24 +/- 9.37     | +1.84e-01 +/- 1.39e+00 |  1.51e+01 +/- 1.0e+00  |  1.49e+01 +/- 9.4e-01  |

YigitElma · 2024-05-21T04:10:21Z

So, the point of this PR is to get rid of repeated code. But if I do so, the test test_data_index_deps fails because we don't have data["phi"] inside compute functions anymore. I can try to add phi in that test. But I want to see all other tests pass first.

codecov · 2024-05-21T16:25:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.24%. Comparing base (56befac) to head (c6792ff).
Report is 2103 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   94.97%   95.24%   +0.27%     
==========================================
  Files          87       87              
  Lines       21822    21633     -189     
==========================================
- Hits        20726    20605     -121     
+ Misses       1096     1028      -68

Files with missing lines	Coverage Δ
desc/coils.py	`96.91% <100.00%> (ø)`
desc/compute/__init__.py	`100.00% <ø> (ø)`
desc/compute/_basis_vectors.py	`100.00% <ø> (+12.88%)`	⬆️
desc/compute/_curve.py	`100.00% <100.00%> (+0.54%)`	⬆️
desc/compute/_surface.py	`100.00% <100.00%> (+2.59%)`	⬆️
desc/compute/data_index.py	`96.66% <100.00%> (ø)`
desc/compute/utils.py	`96.36% <100.00%> (+0.18%)`	⬆️
desc/equilibrium/equilibrium.py	`96.30% <100.00%> (+<0.01%)`	⬆️
desc/geometry/core.py	`95.91% <100.00%> (ø)`
desc/magnetic_fields/_core.py	`96.40% <100.00%> (ø)`
... and 1 more

dpanici · 2024-05-22T19:36:46Z

desc/compute/utils.py

@@ -140,6 +140,16 @@ def _compute(
        data = data_index[parameterization][name]["fun"](
            params=params, transforms=transforms, profiles=profiles, data=data, **kwargs
        )
+        # to reduce repeated code, for 3D and naturally in rtz but desired in xyz basis
+        # quantities can be converted to xyz basis here
+        if (


move this outside of recursive, have _compute functions assume things come in in rpz

dpanici · 2024-05-22T19:44:04Z

Enforce passed-in data is in rpz always (add to docstring)

unalmis · 2024-05-27T19:11:59Z

tests/test_data_index.py

+                #       handle this.
+                assert queried_deps[p][name]["data"].issubset(
+                    data | axis_limit_data
+                ), err_msg


If you can't find a better way, then let's instead subtract phi from the latter set. FYI we removed the tests that ensures everything can be computed individually, i.e. without computing the entire dependency tree, and replaced it with this test which explicitly checks for this via inspecting the source code. For functions like the third derivatives of the basis vectors it's useful to keep this check as strict as possible.

Substracting "phi" may cause problems since we only want to remove it if the quantity is one of the 3d vectors which changed basis in the main compute function.
This was a temporary change. i will think of a better way to do it, once I finish the actual coding part.

I think this should be ok? We're ensuring that the dependencies are a subset of what actually gets computed right? as opposed to ensuring equality?

Actually since we deleted phi from dependencies, the previous version of this test might still be passing.

@YigitElma In that case can you change this back

Nevermind I changed it back on my branch.

desc/compute/utils.py

YigitElma · 2024-05-29T20:24:06Z

Make every compute function to return in rpz by default and make all of the basis changes in main compute function

… not a vector

…r different classes

…nto yge/rpz2xyz

desc/compute/utils.py

desc/magnetic_fields/_core.py

tests/test_compute_funs.py

ddudt

We should wait to get an approval from at least one person who hasn't contributed to this PR.

dpanici · 2024-07-11T08:27:00Z

desc/compute/utils.py

@@ -248,8 +282,8 @@ def get_profiles(keys, obj, grid=None, has_axis=False, jitable=False, **kwargs):
        Grid to compute quantity on.
    has_axis : bool
        Whether the grid to compute on has a node on the magnetic axis.
-    jitable: bool


This was unused?

dpanici · 2024-07-11T08:31:20Z

tests/test_curves.py

@@ -66,7 +66,7 @@ def test_frenet(self):
        c.flip([0, 1, 0])
        c.translate([1, 1, 1])
        data = c.compute(
-            ["frenet_tangent", "frenet_normal", "frenet_binormal"], basis="xyz", grid=0


Interesting that these vectors are the same in xyz and rpz

I'm not confident they were being computed correctly on master

dpanici

Just realized, we should update the change log to reflect this as it's pretty major (that by default the compute function will return things in rpz, as before I think it wasn't always the case?)

Revert changes to tesd_data_index from #1027 https://github.com/PlasmaControl/DESC/pull/1027/files/148d08d97cf6c08b922d0883700b2758dc9b000c#r1704624731

move basis chnage to main recursive _compute function, delete repeate…

f9cd495

…d code

fix inherent basis check

1364bad

YigitElma added 2 commits May 21, 2024 11:44

add comment

eaf136c

fix data index deps test temporarily, need a better check

01f19c7

YigitElma requested review from dpanici, ddudt, f0uriest and unalmis May 21, 2024 18:05

dpanici reviewed May 22, 2024

View reviewed changes

Merge branch 'master' into yge/rpz2xyz

f1d2530

YigitElma marked this pull request as draft May 27, 2024 18:16

Merge branch 'master' into yge/rpz2xyz

148d08d

unalmis reviewed May 27, 2024

View reviewed changes

YigitElma added 4 commits May 28, 2024 15:56

take conversion out of main recursion

ccf43dd

add phi dependency to if statement

a2f3b7f

add x y z vectors as exception

a8bedb6

trying to fix tests

07a1e94

dpanici reviewed May 29, 2024

View reviewed changes

desc/compute/utils.py Outdated Show resolved Hide resolved

just take the lowercase of basis kwarg for consistency

702f132

YigitElma added 6 commits June 5, 2024 11:28

set all curve compute functions to return in rpz

443ceaf

set all surface compute functions to return in rpz

512f81c

update _compute for 'x' conversion, it needs special care since it is…

b433d0c

… not a vector

don't include curve compute functions for this

b87b018

fix typo

1716ba5

fix computing and converting x issue, there are many different 'x' fo…

194f5cd

…r different classes

Merge branch 'yge/rpz2xyz' of https://github.com/PlasmaControl/DESC i…

2028c27

…nto yge/rpz2xyz

f0uriest reviewed Jul 9, 2024

View reviewed changes

desc/compute/utils.py Show resolved Hide resolved

f0uriest reviewed Jul 9, 2024

View reviewed changes

desc/magnetic_fields/_core.py Outdated Show resolved Hide resolved

f0uriest reviewed Jul 9, 2024

View reviewed changes

tests/test_compute_funs.py Outdated Show resolved Hide resolved

daniel-dudt and others added 4 commits July 9, 2024 17:12

make test_compute_everything_work

8820a0a

Merge branch 'master' into yge/rpz2xyz

f5281e5

Add check for invalid kwarg, avoid reusing common argname

375d87e

Fix params left over from merge conflict

986f529

ddudt requested a review from f0uriest July 10, 2024 17:03

f0uriest previously approved these changes Jul 10, 2024

View reviewed changes

dpanici mentioned this pull request Jul 10, 2024

Make internal use of keys and names consistent #1121

Open

f0uriest requested a review from ddudt July 11, 2024 00:01

Merge branch 'master' into yge/rpz2xyz

9f86c2d

ddudt previously approved these changes Jul 11, 2024

View reviewed changes

dpanici approved these changes Jul 11, 2024

View reviewed changes

dpanici requested changes Jul 11, 2024

View reviewed changes

update Changelog

c6792ff

daniel-dudt dismissed stale reviews from ddudt and f0uriest via c6792ff July 11, 2024 15:04

ddudt requested review from dpanici, f0uriest and ddudt July 11, 2024 15:04

ddudt approved these changes Jul 11, 2024

View reviewed changes

dpanici approved these changes Jul 11, 2024

View reviewed changes

f0uriest approved these changes Jul 11, 2024

View reviewed changes

f0uriest merged commit 0fbee18 into master Jul 11, 2024
18 checks passed

f0uriest deleted the yge/rpz2xyz branch July 11, 2024 17:39

dpanici mentioned this pull request Jul 23, 2024

C0 coils with splines #970

Open

unalmis mentioned this pull request Aug 5, 2024

zeta_B computation is sometimes wrong #1163

Closed

unalmis added a commit that referenced this pull request Aug 9, 2024

Update test_data_index for changes in #1000 and

1573297

Revert changes to tesd_data_index from #1027 https://github.com/PlasmaControl/DESC/pull/1027/files/148d08d97cf6c08b922d0883700b2758dc9b000c#r1704624731

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move basis change to main compute function #1027

Move basis change to main compute function #1027

YigitElma commented May 20, 2024 •

edited

Loading

github-actions bot commented May 20, 2024 •

edited

Loading

YigitElma commented May 21, 2024

codecov bot commented May 21, 2024 •

edited

Loading

dpanici May 22, 2024

dpanici commented May 22, 2024

unalmis May 27, 2024 •

edited

Loading

YigitElma May 27, 2024

f0uriest Jul 3, 2024

YigitElma Jul 11, 2024

unalmis Aug 5, 2024

unalmis Aug 5, 2024

YigitElma commented May 29, 2024

ddudt left a comment

dpanici Jul 11, 2024

ddudt Jul 11, 2024

dpanici Jul 11, 2024

ddudt Jul 11, 2024

dpanici left a comment

Move basis change to main compute function #1027

Move basis change to main compute function #1027

Conversation

YigitElma commented May 20, 2024 • edited Loading

github-actions bot commented May 20, 2024 • edited Loading

YigitElma commented May 21, 2024

codecov bot commented May 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dpanici commented May 22, 2024

unalmis May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YigitElma commented May 29, 2024

ddudt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanici left a comment

Choose a reason for hiding this comment

YigitElma commented May 20, 2024 •

edited

Loading

github-actions bot commented May 20, 2024 •

edited

Loading

codecov bot commented May 21, 2024 •

edited

Loading

unalmis May 27, 2024 •

edited

Loading