Dp/requested changes sparse by dpanici · Pull Request #2291 · PlasmaControl/DESC

dpanici · 2026-08-13T18:12:34Z

@unalmis I put my requested changes in this PR pointing to yours, tests locally pass for test_integrals, test_objective_funs and test_compute_everything (paths where I thought warnings might pop up potentially), feel free to merge if you accept these change requests

github-actions · 2026-08-13T18:37:21Z

Memory benchmark result

|               Test Name                |      %Δ      |    Master (MB)     |      PR (MB)       |    Δ (MB)    |    Time PR (s)     |  Time Master (s)   |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
  test_objective_jac_w7x                 |    0.04 %    |     4.240e+03      |     4.242e+03      |     1.68     |       25.58        |       23.17        |
  test_proximal_jac_w7x_with_eq_update   |   -0.86 %    |     6.879e+03      |     6.819e+03      |    -59.41    |       110.29       |       111.34       |
  test_proximal_freeb_jac                |   -0.04 %    |     1.354e+04      |     1.354e+04      |    -4.91     |       71.49        |       69.57        |
  test_proximal_freeb_jac_blocked        |    0.43 %    |     7.861e+03      |     7.896e+03      |    34.11     |       63.66        |       64.21        |
  test_proximal_freeb_jac_batched        |    0.24 %    |     7.849e+03      |     7.867e+03      |    18.45     |       62.42        |       62.19        |
  test_proximal_jac_ripple               |    0.21 %    |     3.773e+03      |     3.781e+03      |     7.98     |       41.33        |       40.46        |
  test_proximal_jac_ripple_bounce1d      |    0.56 %    |     3.785e+02      |     3.806e+02      |     2.11     |        2.56        |        2.56        |
  test_eq_solve                          |   -1.48 %    |     1.840e+03      |     1.813e+03      |    -27.33    |       36.45        |       36.78        |
  test_objective_quadratic_flux_jac      |   -0.41 %    |     2.715e+03      |     2.704e+03      |    -11.07    |       41.88        |       43.03        |

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

codecov · 2026-08-13T18:45:41Z

Codecov Report

❌ Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.35%. Comparing base (ffa8f24) to head (7abb617).

Files with missing lines	Patch %	Lines
desc/integrals/bounce_integral.py	33.33%	2 Missing ⚠️
desc/objectives/_fast_ion.py	50.00%	2 Missing ⚠️
desc/objectives/_neoclassical.py	50.00%	2 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##           ku/sparse_pullback    #2291      +/-   ##
======================================================
- Coverage               94.37%   94.35%   -0.02%     
======================================================
  Files                     101      101              
  Lines                   28944    28953       +9     
======================================================
+ Hits                    27315    27319       +4     
- Misses                   1629     1634       +5

Files with missing lines	Coverage Δ
desc/integrals/_bounce_utils.py	`94.82% <ø> (ø)`
desc/integrals/bounce_integral.py	`97.21% <33.33%> (-0.21%)`	⬇️
desc/objectives/_fast_ion.py	`88.63% <50.00%> (-3.87%)`	⬇️
desc/objectives/_neoclassical.py	`88.37% <50.00%> (-3.94%)`	⬇️

🚀 New features to boost your workflow:

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

ddudt · 2026-08-14T18:58:35Z

In this PR can we also change the name of num_field_periods to field_period_transits as explained in this comment?

ddudt · 2026-08-15T20:17:48Z


    @staticmethod
-    def pitch_quad(min_B, max_B, num_pitch, **kwargs):
+    def get_pitch_inv_quad(min_B, max_B, num_pitch, simp=True):


@unalmis Was there a compelling reason to change the name to pitch_quad? If so, I can revert this and replace it with an alias to throw a deprecation warning about the new function name, like I did for regular_points -> bounce_points. But the original name get_pitch_inv_quad sounds more descriptive so I prefer keeping it.

pitch_quad is shorter. get_pitch_inv_quad still works so there is no need to remove pitch_quad. What do you mean like you did for regular to bounce points? Those were private functions so you don't need deprecation

OK I will keep it as get_pitch_inv_quad if you don't mind.

I mean like this change to throw a warning if you call the old function name. I know we don't really intend for users to call this directly, but it's still good practice and only a few lines of extra code that we can remove after a couple of releases have passed.

Okay on the regular points thing but fyi the whole file is an underscore and there is a docstring at top of file that warns every method here is private and may change without notice.

Just to clarify, in original PR get_pitch_inv_quad still works. Just pitch_quad is added as an option.

unalmis

Looks like there is some confusion that is causing some unnecessary deprecation warnings. Have marked those that should be reverted or excluded.

Also the two line change I added in downstream PR to not include a dummy argument in the docstring should be kept to avoid polluting an already complicated objective. As just a practical example of why that is also useful for developers, observe this PR reverts that change has introduced an inefficiency of allowing jac_chunk_size to be modified by the user. That will result in poor performance at no improvement. I recall I cached the Jacobin so doesn't matter.

unalmis · 2026-08-15T20:27:15Z


    @staticmethod
-    def pitch_quad(min_B, max_B, num_pitch, **kwargs):
+    def get_pitch_inv_quad(min_B, max_B, num_pitch, simp=True):


simp is a private kwarg that should not be documented.

We want to document all arguments, since the documentation is not just for users but also helpful for us developers to remember what things do.

The intention is to always use simp unless user supplies their own quadrature in future. DESC has plenty of methods where some features are buried into kwargs to avoid polluting the signature, especially when those kwargs are unchaged in almost all cases.

unalmis · 2026-08-15T20:48:13Z

revert get_pitch_inv_quad to pitch_quad

unalmis · 2026-08-15T20:48:49Z

        )
        bounce = Bounce1D(grid, data, check=True)
-        pitch_inv, _ = bounce.pitch_quad(
+        pitch_inv, _ = bounce.get_pitch_inv_quad(


unalmis · 2026-08-15T20:48:54Z

        # Exclude singularity not captured by analytic approximation for pitch near
        # the maximum |B|. (This is captured by the numerical integration).
-        pitch_inv = Bounce1D.pitch_quad(np.min(B), np.max(B), 100, simp=False)[0][:-1]
+        pitch_inv = Bounce1D.get_pitch_inv_quad(np.min(B), np.max(B), 100, simp=False)[


unalmis · 2026-08-15T20:49:04Z

            quad=chebgauss1(16),  # this is our own custom chebgauss1
        )
-        pitch_inv, _ = bounce.pitch_quad(
+        pitch_inv, _ = bounce.get_pitch_inv_quad(


…o nufft_eps description

ddudt · 2026-08-18T18:21:18Z

Looks like there is some confusion that is causing some unnecessary deprecation warnings. Have marked those that should be reverted or excluded.

Yes I had misunderstood some things, thanks for the clarification. I now get that you were not introducing any breaking changes to the public API, but those changes should still be documented.

Let's try to make some compromises to get this finished. I reverted my edits to the private functions and I'm fine with those changing. But we still want users to be warned about changes to the public API and those warnings can be removed later.

Once you merge this PR we will merge #2170.

unalmis · 2026-08-18T21:58:51Z

These are not resolved and break code.

But we still want users to be warned about changes to the public API and those warnings can be removed later.

Please see second point on why the edits made here are counter to above statement. There is still incorrect understanding that those comments correct.

dpanici added 2 commits August 13, 2026 13:11

make requested changes

3cf2c95

Merge branch 'ku/sparse_pullback' into dp/requested-changes-sparse

7abb617

dpanici requested a review from unalmis August 13, 2026 18:12

dpanici changed the base branch from master to ku/sparse_pullback August 13, 2026 18:12

ddudt mentioned this pull request Aug 14, 2026

Sparse pullback for big performance gain #2170

Open

6 tasks

daniel-dudt added 14 commits August 15, 2026 14:44

use FutureWarning instead of DeprecationWarning

b6c0f25

re-add jac_chunk_size to objectives

247feb2

deprecation warning for use_bounce1d

f78e622

deprecation warnings for old Bounce1D and Bounce2D kwargs

65c16b9

update nufft_eps docstrings

fbab2eb

deprecation warnings for old BounceXD.batch arguments

05a46a5

deprecate old is_fourier and truncate arguments

f6f3d7a

rename Options to BounceOptions

14d4211

rename Options to BounceOptions

473ab63

deprecate name change from regular_points to bounce_points

d062fc7

revert name change from pitch_quad back to get_pitch_inv_quad

be5774a

restore docstring for simp argument

63d57ae

rename num_field_periods to field_period_transits

29c914a

misc edit to parse_argnem_change

95227e4

ddudt reviewed Aug 15, 2026

View reviewed changes

unalmis requested changes Aug 15, 2026

View reviewed changes

unalmis reviewed Aug 15, 2026

View reviewed changes

Comment thread desc/integrals/_bounce_utils.py

dpanici and others added 2 commits August 17, 2026 12:49

update changelog, add todo items for deriv mode auto, revert change t…

01af6ea

…o nufft_eps description

revert some changes

2e9626d

ddudt mentioned this pull request Aug 18, 2026

Update default deriv_mode of Bounce2D objectives to "auto" once DESC supports JAX >=0.11.0 #2294

Open

daniel-dudt added 2 commits August 18, 2026 11:52

remove jac_chunk_size depcrecation warnings

39ba220

update Changelog formatting

58dffdd

revert formatting changes

d7d446d

ddudt mentioned this pull request Aug 18, 2026

Remove deprecated things from DESC #2051

Open

17 tasks

ddudt requested a review from unalmis August 18, 2026 18:22

unalmis reviewed Aug 18, 2026

View reviewed changes

Comment thread desc/integrals/bounce_integral.py

Uh oh!

Conversation

dpanici commented Aug 13, 2026

Uh oh!

github-actions Bot commented Aug 13, 2026

Memory benchmark result

Uh oh!

codecov Bot commented Aug 13, 2026

Codecov Report

Uh oh!

ddudt commented Aug 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unalmis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unalmis Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ddudt commented Aug 18, 2026

Uh oh!

Uh oh!

unalmis commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

unalmis left a comment •

edited

Loading

unalmis Aug 18, 2026 •

edited

Loading

unalmis commented Aug 18, 2026 •

edited

Loading