Skip to content

Add warning for batched mode when a sub objective has rev mode - #2284

Open
dpanici wants to merge 10 commits into
masterfrom
dp/rev-mode-batched-warn
Open

Add warning for batched mode when a sub objective has rev mode#2284
dpanici wants to merge 10 commits into
masterfrom
dp/rev-mode-batched-warn

Conversation

@dpanici

@dpanici dpanici commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Does not resolve #2243 but at least warns users of the situation occuring

I am not sure if we can do batched as not fwd mode: maybe we can do it as like batched all at once but in reverse mode if all sub-objectives are rev mode.
Either way this will just throw a warning in the case that batched is set as ObjectiveFunction deriv_mode but any sub objective has rev mode, since that indicates a probable inefficiency.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Memory benchmark result

|               Test Name                |      %Δ      |    Master (MB)     |      PR (MB)       |    Δ (MB)    |    Time PR (s)     |  Time Master (s)   |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
  test_objective_jac_w7x                 |   -0.32 %    |     4.253e+03      |     4.239e+03      |    -13.72    |       30.32        |       28.24        |
  test_proximal_jac_w7x_with_eq_update   |   -0.20 %    |     6.876e+03      |     6.863e+03      |    -13.47    |       154.28       |       150.54       |
  test_proximal_freeb_jac                |   -0.07 %    |     1.355e+04      |     1.354e+04      |    -10.04    |       79.85        |       79.08        |
  test_proximal_freeb_jac_blocked        |   -0.20 %    |     7.899e+03      |     7.883e+03      |    -15.69    |       69.18        |       68.73        |
  test_proximal_freeb_jac_batched        |    0.09 %    |     7.881e+03      |     7.889e+03      |     7.11     |       67.98        |       69.27        |
  test_proximal_jac_ripple               |   -3.14 %    |     3.808e+03      |     3.688e+03      |   -119.69    |       52.79        |       53.30        |
  test_proximal_jac_ripple_bounce1d      |    1.27 %    |     3.962e+03      |     4.013e+03      |    50.41     |       66.54        |       67.18        |
  test_eq_solve                          |    1.28 %    |     1.819e+03      |     1.842e+03      |    23.24     |       51.46        |       52.01        |
  test_objective_quadratic_flux_jac      |   -0.09 %    |     2.712e+03      |     2.710e+03      |    -2.39     |       52.36        |       52.89        |

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

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The message was not very clear, and there was a typo or may not even support reverse mode at all should be forward mode.

Also, while we are at it, would we like to print something like this at the end?

if verbose > 1:
    print(f"{self.name} deriv_mode : {self._deriv_mode}")
    if self._deriv_mode == "batched":
        print(f"{self.name} jac_chunk_size: {self._jac_chunk_size}")
    else:
        for o in self.objectives:
             print(f"{o.name} jac_chunk_size: {o._jac_chunk_size}")
    timer.disp("Objective build")

Should also fix the failing test

Comment thread desc/objectives/objective_funs.py Outdated
@YigitElma YigitElma added the skip_changelog No need to update changelog on this PR label Aug 8, 2026
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.38%. Comparing base (33076c8) to head (20fcec1).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2284   +/-   ##
=======================================
  Coverage   94.37%   94.38%           
=======================================
  Files         101      101           
  Lines       28953    28960    +7     
=======================================
+ Hits        27325    27333    +8     
+ Misses       1628     1627    -1     
Files with missing lines Coverage Δ
desc/objectives/objective_funs.py 95.41% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dpanici dpanici added the run_benchmarks Run timing benchmarks on this PR against current master branch label Aug 10, 2026
@dpanici
dpanici requested a review from YigitElma August 10, 2026 17:03
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -0.40 +/- 8.49     | -3.42e-03 +/- 7.34e-02 |  8.61e-01 +/- 3.4e-02  |  8.65e-01 +/- 6.5e-02  |
 test_equilibrium_init_lowres            |     -0.23 +/- 4.01     | -1.53e-02 +/- 2.63e-01 |  6.53e+00 +/- 1.3e-01  |  6.55e+00 +/- 2.3e-01  |
 test_objective_compile_atf              |     -0.15 +/- 3.95     | -9.43e-03 +/- 2.44e-01 |  6.17e+00 +/- 1.5e-01  |  6.18e+00 +/- 1.9e-01  |
 test_objective_compute_atf              |     -3.82 +/- 18.25    | -8.31e-05 +/- 3.97e-04 |  2.09e-03 +/- 2.4e-04  |  2.18e-03 +/- 3.2e-04  |
 test_objective_jac_atf                  |     -2.11 +/- 4.65     | -3.37e-02 +/- 7.43e-02 |  1.56e+00 +/- 6.1e-02  |  1.60e+00 +/- 4.3e-02  |
 test_perturb_1                          |     +2.56 +/- 2.98     | +3.04e-01 +/- 3.54e-01 |  1.22e+01 +/- 3.1e-01  |  1.19e+01 +/- 1.7e-01  |
 test_proximal_jac_atf                   |     -0.81 +/- 1.16     | -4.28e-02 +/- 6.13e-02 |  5.26e+00 +/- 4.7e-02  |  5.30e+00 +/- 4.0e-02  |
 test_proximal_freeb_compute             |     -2.04 +/- 2.84     | -3.42e-03 +/- 4.77e-03 |  1.64e-01 +/- 3.7e-03  |  1.68e-01 +/- 3.1e-03  |
 test_solve_fixed_iter                   |     +3.20 +/- 3.54     | +7.83e-01 +/- 8.66e-01 |  2.53e+01 +/- 6.7e-01  |  2.45e+01 +/- 5.5e-01  |
 test_LinearConstraintProjection_build   |     +4.52 +/- 3.74     | +3.15e-01 +/- 2.61e-01 |  7.29e+00 +/- 2.4e-01  |  6.98e+00 +/- 9.4e-02  |
 test_objective_compute_ripple           |     -1.70 +/- 4.55     | -3.98e-03 +/- 1.07e-02 |  2.30e-01 +/- 7.3e-03  |  2.34e-01 +/- 7.8e-03  |
 test_objective_grad_ripple              |     -0.60 +/- 3.96     | -5.42e-03 +/- 3.56e-02 |  8.93e-01 +/- 1.9e-02  |  8.98e-01 +/- 3.0e-02  |
 test_objective_quadratic_flux_compute   |     +2.17 +/- 15.09    | +1.12e-03 +/- 7.81e-03 |  5.29e-02 +/- 6.8e-03  |  5.18e-02 +/- 3.8e-03  |
 test_build_transform_fft_midres         |     -4.59 +/- 2.76     | -4.22e-02 +/- 2.53e-02 |  8.76e-01 +/- 1.1e-02  |  9.18e-01 +/- 2.3e-02  |
 test_build_transform_fft_highres        |     -3.60 +/- 2.63     | -4.35e-02 +/- 3.18e-02 |  1.17e+00 +/- 2.8e-02  |  1.21e+00 +/- 1.6e-02  |
 test_equilibrium_init_medres            |     -5.01 +/- 2.89     | -3.59e-01 +/- 2.07e-01 |  6.81e+00 +/- 1.5e-01  |  7.17e+00 +/- 1.4e-01  |
 test_objective_compile_dshape_current   |     -1.28 +/- 2.73     | -5.39e-02 +/- 1.15e-01 |  4.17e+00 +/- 1.1e-01  |  4.23e+00 +/- 2.5e-02  |
 test_objective_compute_dshape_current   |     +4.88 +/- 13.76    | +3.27e-05 +/- 9.21e-05 |  7.02e-04 +/- 7.9e-05  |  6.70e-04 +/- 4.8e-05  |
 test_objective_jac_dshape_current       |     -1.45 +/- 24.25    | -3.50e-04 +/- 5.85e-03 |  2.38e-02 +/- 4.2e-03  |  2.41e-02 +/- 4.1e-03  |
 test_perturb_2                          |     -3.63 +/- 1.62     | -5.78e-01 +/- 2.58e-01 |  1.53e+01 +/- 1.6e-01  |  1.59e+01 +/- 2.0e-01  |
 test_proximal_jac_atf_with_eq_update    |     -0.30 +/- 1.22     | -3.62e-02 +/- 1.48e-01 |  1.21e+01 +/- 6.8e-02  |  1.21e+01 +/- 1.3e-01  |
 test_proximal_freeb_jac                 |     -1.63 +/- 3.03     | -7.90e-02 +/- 1.47e-01 |  4.77e+00 +/- 1.2e-01  |  4.85e+00 +/- 8.3e-02  |
 test_solve_fixed_iter_compiled          |     -2.76 +/- 2.16     | -1.69e-01 +/- 1.32e-01 |  5.94e+00 +/- 8.9e-02  |  6.11e+00 +/- 9.8e-02  |
 test_objective_compute_ripple_bounce1d  |     -2.73 +/- 3.72     | -8.47e-03 +/- 1.15e-02 |  3.02e-01 +/- 3.1e-03  |  3.10e-01 +/- 1.1e-02  |
 test_objective_grad_ripple_bounce1d     |     +0.91 +/- 2.11     | +8.62e-03 +/- 1.99e-02 |  9.52e-01 +/- 1.7e-02  |  9.43e-01 +/- 1.1e-02  |
 test_objective_quadratic_flux_jac       |     -0.32 +/- 0.50     | -2.72e-02 +/- 4.23e-02 |  8.43e+00 +/- 3.9e-02  |  8.46e+00 +/- 1.6e-02  |

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

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Other then the changelog issue it is fine.

Comment thread CHANGELOG.md Outdated
@YigitElma
YigitElma requested review from a team, IssraAli, YigitElma, ddudt, f0uriest, lkadz, rahulgaur104 and singh-jaydeep and removed request for a team August 13, 2026 16:33
self._deriv_mode = "blocked"

rev_objs = [
o.__class__.__name__ for o in self.objectives if o._deriv_mode == "rev"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

suggestion(if-minor)
I would prefer o.name, since it would be more clear if the user has multiple of the same objectives and gave them unique names. Also because I think we typically use obj.name instead of the class name when printing objectives.

self._deriv_mode = "blocked"

rev_objs = [
o.__class__.__name__ for o in self.objectives if o._deriv_mode == "rev"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
o.__class__.__name__ for o in self.objectives if o._deriv_mode == "rev"
o.name for o in self.objectives if o._deriv_mode == "rev"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run_benchmarks Run timing benchmarks on this PR against current master branch skip_changelog No need to update changelog on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix deriv mode = batched

3 participants