Switch compute from recursive to looped by f0uriest · Pull Request #1557 · PlasmaControl/DESC

f0uriest · 2025-02-02T03:08:35Z

This PR builds upon #2156

_compute now uses a for loop instead of recursion
- We still need to call _get_deps to consider given data
Cleans up data dependency getters and removes the redundant functions

Resolves #1336

github-actions · 2025-02-02T03:39:08Z

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -0.34 +/- 3.49     | -2.92e-03 +/- 2.96e-02 |  8.46e-01 +/- 2.1e-02  |  8.49e-01 +/- 2.1e-02  |
 test_equilibrium_init_medres            |     -1.29 +/- 3.98     | -8.88e-02 +/- 2.74e-01 |  6.79e+00 +/- 2.0e-01  |  6.88e+00 +/- 1.9e-01  |
 test_equilibrium_init_highres           |     -4.75 +/- 5.67     | -3.77e-01 +/- 4.50e-01 |  7.56e+00 +/- 1.5e-01  |  7.94e+00 +/- 4.2e-01  |
 test_objective_compile_dshape_current   |     -1.11 +/- 2.61     | -4.69e-02 +/- 1.10e-01 |  4.16e+00 +/- 9.1e-02  |  4.21e+00 +/- 6.1e-02  |
 test_objective_compute_dshape_current   |     +1.92 +/- 14.08    | +1.44e-05 +/- 1.06e-04 |  7.67e-04 +/- 7.9e-05  |  7.52e-04 +/- 7.1e-05  |
 test_objective_jac_dshape_current       |     -4.12 +/- 17.23    | -9.76e-04 +/- 4.08e-03 |  2.27e-02 +/- 2.3e-03  |  2.37e-02 +/- 3.3e-03  |
 test_perturb_2                          |     -3.64 +/- 2.32     | -7.35e-01 +/- 4.68e-01 |  1.94e+01 +/- 3.1e-01  |  2.02e+01 +/- 3.5e-01  |
 test_proximal_jac_atf_with_eq_update    |     -0.17 +/- 1.14     | -2.00e-02 +/- 1.37e-01 |  1.19e+01 +/- 1.3e-01  |  1.20e+01 +/- 4.9e-02  |
 test_proximal_freeb_jac                 |     -0.06 +/- 3.04     | -2.95e-03 +/- 1.45e-01 |  4.76e+00 +/- 1.2e-01  |  4.77e+00 +/- 8.4e-02  |
 test_solve_fixed_iter_compiled          |     +1.24 +/- 2.91     | +7.81e-02 +/- 1.83e-01 |  6.38e+00 +/- 1.1e-01  |  6.30e+00 +/- 1.5e-01  |
 test_LinearConstraintProjection_build   |     -3.22 +/- 3.13     | -2.25e-01 +/- 2.18e-01 |  6.76e+00 +/- 1.4e-01  |  6.98e+00 +/- 1.7e-01  |
 test_objective_compute_ripple_bounce1d  |     -1.05 +/- 2.55     | -3.20e-03 +/- 7.74e-03 |  3.01e-01 +/- 6.9e-03  |  3.04e-01 +/- 3.6e-03  |
 test_objective_grad_ripple_bounce1d     |     +0.59 +/- 2.21     | +5.49e-03 +/- 2.05e-02 |  9.33e-01 +/- 1.8e-02  |  9.27e-01 +/- 1.0e-02  |
 test_build_transform_fft_midres         |     -3.12 +/- 3.88     | -2.91e-02 +/- 3.61e-02 |  9.02e-01 +/- 1.9e-02  |  9.31e-01 +/- 3.1e-02  |
 test_build_transform_fft_highres        |     -2.06 +/- 3.00     | -2.50e-02 +/- 3.64e-02 |  1.19e+00 +/- 2.5e-02  |  1.21e+00 +/- 2.7e-02  |
 test_equilibrium_init_lowres            |     -2.32 +/- 3.23     | -1.59e-01 +/- 2.20e-01 |  6.67e+00 +/- 1.4e-01  |  6.83e+00 +/- 1.7e-01  |
 test_objective_compile_atf              |     +0.23 +/- 4.25     | +1.50e-02 +/- 2.73e-01 |  6.44e+00 +/- 1.9e-01  |  6.42e+00 +/- 2.0e-01  |
 test_objective_compute_atf              |    +16.61 +/- 11.24    | +3.46e-04 +/- 2.34e-04 |  2.43e-03 +/- 2.0e-04  |  2.08e-03 +/- 1.2e-04  |
 test_objective_jac_atf                  |     -2.97 +/- 4.93     | -4.95e-02 +/- 8.22e-02 |  1.62e+00 +/- 5.6e-02  |  1.67e+00 +/- 6.0e-02  |
 test_perturb_1                          |     +2.90 +/- 7.05     | +4.79e-01 +/- 1.16e+00 |  1.70e+01 +/- 1.1e+00  |  1.65e+01 +/- 2.1e-01  |
 test_proximal_jac_atf                   |     +0.89 +/- 1.61     | +4.75e-02 +/- 8.57e-02 |  5.38e+00 +/- 8.1e-02  |  5.33e+00 +/- 2.8e-02  |
 test_proximal_freeb_compute             |     +3.05 +/- 2.68     | +5.16e-03 +/- 4.54e-03 |  1.75e-01 +/- 3.1e-03  |  1.70e-01 +/- 3.3e-03  |
 test_solve_fixed_iter                   |     +2.19 +/- 3.38     | +5.51e-01 +/- 8.51e-01 |  2.58e+01 +/- 7.7e-01  |  2.52e+01 +/- 3.6e-01  |
 test_objective_compute_ripple           |     +0.41 +/- 5.34     | +9.81e-04 +/- 1.28e-02 |  2.41e-01 +/- 1.1e-02  |  2.40e-01 +/- 6.5e-03  |
 test_objective_grad_ripple              |     +1.86 +/- 2.61     | +1.65e-02 +/- 2.31e-02 |  9.02e-01 +/- 1.6e-02  |  8.86e-01 +/- 1.7e-02  |

Github CI performance can be noisy. When evaluating the benchmarks, developers should take this into account.

YigitElma · 2026-04-18T16:07:40Z

The logic should take into account that a given data may make certain sub dependencies obselete

…from before

… always have these keys in, some clean up

YigitElma · 2026-04-19T05:05:49Z

        Type of object to compute for, eg Equilibrium, Curve, etc.
    names : str or array-like of str
        Name(s) of the quantity(s) to compute.
-    deps : set[str]


Previously this function was recursive, so this deps was needed but not anymore

YigitElma · 2026-04-19T05:07:24Z

-
-    def _get_derivs_1_key(key):
-        if has_axis:
-            if "full_with_axis_dependencies" in data_index[p][key]:


We used to call this function in _build_data_index, but now data_index is already built when we call this function

github-actions · 2026-04-19T05:25:10Z

Memory benchmark result

|               Test Name                |      %Δ      |    Master (MB)     |      PR (MB)       |    Δ (MB)    |    Time PR (s)     |  Time Master (s)   |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
  test_objective_jac_w7x                 |    3.10 %    |     3.941e+03      |     4.064e+03      |    122.30    |       33.36        |       30.45        |
  test_proximal_jac_w7x_with_eq_update   |   -1.61 %    |     6.634e+03      |     6.527e+03      |   -106.80    |       153.60       |       159.59       |
  test_proximal_freeb_jac                |   -0.01 %    |     1.337e+04      |     1.337e+04      |    -1.38     |       81.35        |       81.44        |
  test_proximal_freeb_jac_blocked        |    0.33 %    |     7.676e+03      |     7.701e+03      |    25.57     |       70.81        |       71.92        |
  test_proximal_freeb_jac_batched        |   -0.17 %    |     7.664e+03      |     7.651e+03      |    -12.80    |       71.61        |       71.63        |
  test_proximal_jac_ripple               |   -0.18 %    |     3.516e+03      |     3.510e+03      |    -6.34     |       55.37        |       54.86        |
  test_proximal_jac_ripple_bounce1d      |   -1.01 %    |     3.784e+03      |     3.745e+03      |    -38.38    |       70.96        |       69.04        |
  test_eq_solve                          |    2.01 %    |     1.924e+03      |     1.963e+03      |    38.77     |       61.42        |       59.92        |

For the memory plots, go to the summary of Memory Benchmarks workflow and download the artifact.

codecov · 2026-04-30T04:58:37Z

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.38%. Comparing base (24aa7b9) to head (3a6e8c7).

Files with missing lines	Patch %	Lines
desc/compute/utils.py	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1557      +/-   ##
==========================================
+ Coverage   94.32%   94.38%   +0.05%     
==========================================
  Files         101      101              
  Lines       28866    28829      -37     
==========================================
- Hits        27228    27209      -19     
+ Misses       1638     1620      -18

Files with missing lines	Coverage Δ
desc/compute/__init__.py	`100.00% <100.00%> (ø)`
desc/compute/data_index.py	`95.94% <100.00%> (+0.05%)`	⬆️
desc/compute/utils.py	`96.59% <98.00%> (+7.32%)`	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

YigitElma · 2026-04-30T05:40:06Z

I am not sure if this PR is necessary, given that there is no performance difference. I guess it is cleaner than before, anyway it is ready for review.

![PR Age](https://img.shields.io/github/issues/detail/age/PlasmaControl/DESC/2157?label=Ready%20for%20merge%20since) - Adds FFT grids and field aligned grids to `test_compute_everything`. Should be merged before stuff that changes grids #2053 or stuff that changes how the dependencies are ordered/selected for computation #1557 to ensure they do not cause bugs in the computations. - Deletes a bunch of files that are now not necessary and removes bounce objective docstring boilerplate - uses `nondiff_argnums` instead of `nondiff_argnames` so that the code works on old jax versions - Adds comments to explain per bounce_comments PR - Resolves #2162 . --------- Co-authored-by: Rory Conlin <mail@fouriest.net> Co-authored-by: Dario Panici <37969854+dpanici@users.noreply.github.com>

ddudt

need to fix the failing test, otherwise this looks good

YigitElma · 2026-05-14T22:00:50Z

need to fix the failing test

Btw, I cannot recreate the failing test locally.

YigitElma · 2026-05-28T17:27:29Z

Try fixing the iota profile instead of the bounds
Maybe also remove the bounds for the force balance constraint too

YigitElma · 2026-05-29T13:29:25Z

@f0uriest let me know if you approve, since you opened the PR, I can approve for you

YigitElma · 2026-07-01T09:38:44Z

 @pytest.mark.optimize
 def test_constrained_AL_lsq():
    """Tests that the least squares augmented Lagrangian optimizer does something."""
+    # Note: This test is known to be sensitive to small numerical differences


Related comment about this test
#2247 (comment)

YigitElma · 2026-07-13T23:17:10Z

@dpanici can you check this once you have some time?

Switch compute from recursive to looped

12a8e53

dpanici added 2 commits March 26, 2025 15:27

Merge branch 'master' into rc/compute_tiers

0732219

Merge branch 'master' into rc/compute_tiers

75a72cf

dpanici added the run_benchmarks Run timing benchmarks on this PR against current master branch label Apr 9, 2026

unalmis mentioned this pull request Apr 10, 2026

Add fft grid and raz grid to test against master #2157

Merged

YigitElma reviewed Apr 11, 2026

View reviewed changes

Comment thread desc/compute/utils.py Outdated

YigitElma mentioned this pull request Apr 16, 2026

Improve import time #2156

Merged

YigitElma and others added 4 commits April 17, 2026 15:22

Merge branch 'master' into rc/compute_tiers

e0fff1d

use topological order computed in build_data_index instead of tiers

6a66f85

fix _compute, remove set_tiers

a75ec0c

move topo_order to data_index

e5916d2

YigitElma and others added 5 commits April 19, 2026 00:01

remove recursion in _get_deps, remove redundant deps, fix data issue …

ab47753

…from before

remove _get_deps_1_key and other redundant functions, data_index will…

1a959bc

… always have these keys in, some clean up

minor comments

abff730

skip extra get_data_deps calls, remove redundant functions

377a30e

Merge branch 'master' into rc/compute_tiers

662b157

YigitElma reviewed Apr 19, 2026

View reviewed changes

Comment thread desc/compute/__init__.py

YigitElma added 2 commits April 19, 2026 12:18

Merge branch 'master' into rc/compute_tiers

293bcba

Merge branch 'master' into rc/compute_tiers

1a49974

YigitElma marked this pull request as ready for review April 30, 2026 05:40

YigitElma added the skip_changelog No need to update changelog on this PR label Apr 30, 2026

YigitElma requested review from ddudt and dpanici April 30, 2026 05:41

Merge branch 'master' into rc/compute_tiers

54efa1b

Merge branch 'master' into rc/compute_tiers

5b4ee74

ddudt reviewed May 7, 2026

View reviewed changes

Comment thread desc/compute/__init__.py

try more interesting objective

be2728b

YigitElma and others added 2 commits May 29, 2026 01:35

try to fix the test

2151b83

Merge branch 'master' into rc/compute_tiers

e3473a6

YigitElma requested review from YigitElma, ddudt, dpanici and rahulgaur104 and removed request for dpanici May 29, 2026 13:28

YigitElma and others added 2 commits June 10, 2026 22:54

Merge branch 'master' into rc/compute_tiers

329032f

Merge branch 'master' into rc/compute_tiers

0fb61d9

ddudt approved these changes Jun 25, 2026

View reviewed changes

Merge branch 'master' into rc/compute_tiers

e104579

YigitElma reviewed Jul 1, 2026

View reviewed changes

YigitElma added the easy Short and simple to code or review label Jul 9, 2026

Merge branch 'master' into rc/compute_tiers

3a6e8c7

Uh oh!

Conversation

f0uriest commented Feb 2, 2025 • edited by YigitElma Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

YigitElma commented Apr 18, 2026

Uh oh!

YigitElma Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

YigitElma Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Memory benchmark result

Uh oh!

codecov Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

YigitElma commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddudt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YigitElma commented May 14, 2026

Uh oh!

YigitElma commented May 28, 2026

Uh oh!

YigitElma commented May 29, 2026

Uh oh!

YigitElma Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

YigitElma commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

f0uriest commented Feb 2, 2025 •

edited by YigitElma

Loading

github-actions Bot commented Feb 2, 2025 •

edited

Loading

github-actions Bot commented Apr 19, 2026 •

edited

Loading

codecov Bot commented Apr 30, 2026 •

edited

Loading

YigitElma commented Apr 30, 2026 •

edited

Loading