Skip to content

Fix tests for older dependencies - #2287

Open
YigitElma wants to merge 14 commits into
masterfrom
yge/fix-compute-everything-jax-finufft
Open

Fix tests for older dependencies#2287
YigitElma wants to merge 14 commits into
masterfrom
yge/fix-compute-everything-jax-finufft

Conversation

@YigitElma

@YigitElma YigitElma commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

See #2286 (comment)

The failing unit test is related to jax-finufft version. It is similar to the one we faced in #2264. This commit aimed to solve the issue but it didn't and the later commit deleted a unit test which in return moved the failing test to a different Python version that uses a newer jax-finufft (the test passed as if the original commit solved the issue). Anyway, I can fix this in a separate PR, the changes in this PR shouldn't affect Gamma_c at all.

  • I temporarily changed all unit and regression tests to use 3.10, so that all use jax-finufft 1.2.0. Everything passes now.

Optional

Some of the most sensitive tests like test_solve_bounds, test_constrained_AL_lsq, etc were creating singular Jacobians (I checked the ranks) during optimization, which is something that the default QR cannot handle. This is caused by the bounds, if the objective is inside the bounds that row is full of 0s, and if there are enough rows like that, matrix becomes singular. I added a warning for this scenario (we can discuss if we want this or not). It looks at the bounded objectives and checks if they can cause rank deficiency (this is done using static info, so should be jittable for #1669). It only triggers if a user didn't give tr_method explicitly and the default is QR.

@github-actions

github-actions Bot commented Aug 10, 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.58 %    |     4.236e+03      |     4.261e+03      |    24.72     |       31.09        |       26.30        |
  test_proximal_jac_w7x_with_eq_update   |    0.81 %    |     6.806e+03      |     6.861e+03      |    54.96     |       127.24       |       128.81       |
  test_proximal_freeb_jac                |   -0.01 %    |     1.354e+04      |     1.354e+04      |    -0.82     |       78.95        |       83.12        |
  test_proximal_freeb_jac_blocked        |    0.04 %    |     7.858e+03      |     7.861e+03      |     2.99     |       72.18        |       73.86        |
  test_proximal_freeb_jac_batched        |    0.11 %    |     7.870e+03      |     7.878e+03      |     8.74     |       70.38        |       73.04        |
  test_proximal_jac_ripple               |   -0.12 %    |     3.724e+03      |     3.720e+03      |    -4.36     |       47.32        |       47.90        |
  test_proximal_jac_ripple_bounce1d      |    0.98 %    |     3.936e+03      |     3.974e+03      |    38.39     |       58.48        |       58.49        |
  test_eq_solve                          |   -0.31 %    |     1.843e+03      |     1.837e+03      |    -5.78     |       43.02        |       43.33        |
  test_objective_quadratic_flux_jac      |    0.17 %    |     1.884e+03      |     1.888e+03      |     3.19     |       30.78        |       31.35        |

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

@YigitElma YigitElma changed the title Fix jax-finufft related error in test_compute_everything Fix tests for older dependencies Aug 10, 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.35%. Comparing base (7582fb3) to head (cd40f7d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2287   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files         101      101           
  Lines       29036    29055   +19     
=======================================
+ Hits        27396    27415   +19     
  Misses       1640     1640           
Files with missing lines Coverage Δ
desc/optimize/_desc_wrappers.py 92.68% <100.00%> (+1.50%) ⬆️
desc/optimize/aug_lagrangian_ls.py 96.00% <ø> (ø)
desc/optimize/least_squares.py 99.45% <ø> (ø)
desc/optimize/utils.py 95.52% <100.00%> (+0.09%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YigitElma YigitElma added the skip_changelog No need to update changelog on this PR label Aug 11, 2026
@YigitElma
YigitElma marked this pull request as ready for review August 11, 2026 16:44
@YigitElma
YigitElma requested review from a team, ddudt, dpanici, f0uriest, lkadz, rahulgaur104, singh-jaydeep and unalmis and removed request for a team August 11, 2026 16:45
ddudt
ddudt previously approved these changes Aug 13, 2026
@YigitElma YigitElma mentioned this pull request Aug 15, 2026
Comment thread desc/optimize/utils.py

``potrf`` only errors out, and so returns nan, when a pivot comes out
non-positive. A pivot at the noise level instead completes but gives a factor that
is useless to solve with, and which of the two a marginal matrix gets depends on

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

For example, test_multiobject_optimization_al was creating such matrices, failing on CI for #2293, but not locally

@YigitElma
YigitElma requested a review from ddudt August 18, 2026 02:46
@YigitElma YigitElma removed the skip_changelog No need to update changelog on this PR label Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants