Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up testing #930

Merged
merged 42 commits into from
Apr 4, 2024
Merged

Speed up testing #930

merged 42 commits into from
Apr 4, 2024

Conversation

f0uriest
Copy link
Member

@f0uriest f0uriest commented Mar 6, 2024

  • Removes some redundant / unneeded tests
  • Refactors some big tests to use pytest.mark.parametrize for more efficient parallelization
  • Relabels some slow unit tests as regression tests
  • Removes fixtures that require solve/optimize from as many tests as possible - replace with precomputed desc.examples
  • Reduces resolution where possible

More can definitely be done, even parallelized unit tests still take ~30 mins, which I'd like to get down to at least half that, but that can be another PR.

@f0uriest f0uriest marked this pull request as draft March 6, 2024 22:48
@@ -1200,54 +1200,6 @@ def test_BootstrapRedlConsistency_normalization(self):
# Results are not perfectly identical because ln(Lambda) is not quite invariant.
np.testing.assert_allclose(results, expected, rtol=2e-3)

@pytest.mark.unit
@pytest.mark.solve
def test_BootstrapRedlConsistency_resolution(self, DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

this is redundant with test_objective_funs.test_compute_scalar_resolution

@@ -78,34 +78,6 @@ def test_compute_theta_coords(DSHAPE_current):

@pytest.mark.unit
def test_map_coordinates():
"""Test root finding for (rho,theta,zeta) from (R,phi,Z)."""
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with the other test_map_coordinates

@@ -626,56 +624,6 @@ def test_derivative_modes():
np.testing.assert_allclose(H1, H3, atol=1e-10)


@pytest.mark.unit
@pytest.mark.xfail
def test_rejit():
Copy link
Member Author

Choose a reason for hiding this comment

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

changing an objective after creation is no longer recommended.


# this would fail before v0.8.2 when trying to get objective.x
eq.change_resolution(L=5, M=5)
obj.build(eq)
Copy link
Member Author

Choose a reason for hiding this comment

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

build method no longer even accepts eq, this is basically passing use_jit=eq

@@ -1935,88 +1859,115 @@ def test_compute_scalar_resolution(): # noqa: C901
np.testing.assert_allclose(f, f[-1], rtol=1e-2)


@pytest.mark.unit
def test_objective_no_nangrad():
Copy link
Member Author

Choose a reason for hiding this comment

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

this was just refactored into smaller tests to help parallelization

@@ -559,38 +559,50 @@ def test_wrappers():
np.testing.assert_allclose(ob.weights, con[0].weight)


@pytest.mark.unit
@pytest.mark.slow
def test_all_optimizers():
Copy link
Member Author

Choose a reason for hiding this comment

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

this was just refactored into smaller tests to help parallelization

@pytest.mark.slow
@pytest.mark.unit
@pytest.mark.optimize
def test_proximal_with_PlasmaVesselDistance():
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with test_examples.test_multiobject_optimization_prox

Copy link
Contributor

github-actions bot commented Mar 7, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +6.87 +/- 3.86     | +1.19e-01 +/- 6.69e-02 |  1.85e+00 +/- 5.3e-02  |  1.73e+00 +/- 4.0e-02  |
 test_build_transform_fft_midres         |     +3.59 +/- 3.28     | +6.97e-02 +/- 6.37e-02 |  2.01e+00 +/- 6.1e-02  |  1.94e+00 +/- 1.9e-02  |
 test_build_transform_fft_highres        |     +1.96 +/- 2.43     | +4.56e-02 +/- 5.67e-02 |  2.38e+00 +/- 5.3e-02  |  2.33e+00 +/- 2.1e-02  |
 test_equilibrium_init_lowres            |     +2.06 +/- 2.17     | +1.94e-01 +/- 2.05e-01 |  9.63e+00 +/- 1.7e-01  |  9.44e+00 +/- 1.1e-01  |
 test_equilibrium_init_medres            |     -1.13 +/- 2.03     | -1.16e-01 +/- 2.07e-01 |  1.01e+01 +/- 1.8e-01  |  1.02e+01 +/- 9.9e-02  |
 test_equilibrium_init_highres           |     +3.58 +/- 3.34     | +4.23e-01 +/- 3.95e-01 |  1.22e+01 +/- 3.5e-01  |  1.18e+01 +/- 1.8e-01  |
 test_objective_compile_dshape_current   |     -5.66 +/- 9.79     | -2.10e-01 +/- 3.62e-01 |  3.49e+00 +/- 2.5e-01  |  3.70e+00 +/- 2.6e-01  |
 test_objective_compile_atf              |     -0.03 +/- 1.48     | -2.17e-03 +/- 1.06e-01 |  7.17e+00 +/- 9.9e-02  |  7.17e+00 +/- 4.0e-02  |
 test_objective_compute_dshape_current   |     +4.90 +/- 2.51     | +1.90e-04 +/- 9.73e-05 |  4.06e-03 +/- 8.0e-05  |  3.87e-03 +/- 5.5e-05  |
 test_objective_compute_atf              |     +1.92 +/- 2.85     | +3.34e-04 +/- 4.94e-04 |  1.77e-02 +/- 3.8e-04  |  1.74e-02 +/- 3.2e-04  |
 test_objective_jac_dshape_current       |     +8.38 +/- 6.60     | +3.33e-03 +/- 2.62e-03 |  4.30e-02 +/- 1.9e-03  |  3.97e-02 +/- 1.8e-03  |
 test_objective_jac_atf                  |     +1.88 +/- 2.89     | +3.49e-02 +/- 5.38e-02 |  1.90e+00 +/- 4.2e-02  |  1.86e+00 +/- 3.4e-02  |
 test_perturb_1                          |     -0.38 +/- 1.94     | -5.41e-02 +/- 2.73e-01 |  1.40e+01 +/- 9.9e-02  |  1.41e+01 +/- 2.6e-01  |
 test_perturb_2                          |     -0.23 +/- 3.03     | -4.36e-02 +/- 5.75e-01 |  1.89e+01 +/- 4.2e-01  |  1.90e+01 +/- 3.9e-01  |
 test_proximal_jac_atf                   |     +0.64 +/- 1.39     | +4.53e-02 +/- 9.87e-02 |  7.17e+00 +/- 7.3e-02  |  7.13e+00 +/- 6.7e-02  |
 test_proximal_freeb_compute             |     +0.57 +/- 0.73     | +6.45e-04 +/- 8.28e-04 |  1.14e-01 +/- 4.6e-04  |  1.13e-01 +/- 6.9e-04  |
 test_proximal_freeb_jac                 |     +0.13 +/- 1.49     | +9.03e-03 +/- 1.08e-01 |  7.20e+00 +/- 5.6e-02  |  7.19e+00 +/- 9.2e-02  |

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.36%. Comparing base (615de7f) to head (56a7c4c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   95.02%   95.36%   +0.33%     
==========================================
  Files          87       87              
  Lines       21862    21691     -171     
==========================================
- Hits        20775    20685      -90     
+ Misses       1087     1006      -81     
Files Coverage Δ
desc/plotting.py 95.95% <ø> (+6.18%) ⬆️

... and 11 files with indirect coverage changes



@pytest.mark.unit
def test_compare_quantities_to_vmec():
Copy link
Member Author

Choose a reason for hiding this comment

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

already covered with test_vmec_save

@@ -1166,47 +1146,6 @@ def test_boozer_transform(DSHAPE_current):
)


@pytest.mark.unit
def test_compute_grad_p_volume_avg():
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure what this is really testing?

@@ -22,42 +21,10 @@


@pytest.mark.unit
@pytest.mark.solve
def test_compute_geometry(DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with test_vmec_save

@@ -325,34 +261,6 @@ def test_resolution():
assert eq1.R_basis.NFP == 5


@pytest.mark.unit
def test_symmetry():
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with other test_eq_change_symmetry



@pytest.mark.unit
def test_generic_compute():
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with other tests in this file



@pytest.mark.unit
def test_force_balance_axis_error():
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with test_axis_limits.py::test_reverse_mode_ad_axis

@pytest.mark.unit
@pytest.mark.solve
@pytest.mark.mpl_image_compare(remove_text=True, tolerance=tol_1d)
def test_fsa_G(DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with test_fsa_I

Copy link
Collaborator

Choose a reason for hiding this comment

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

If anyone else was initially uncomfortable with removing these tests, I'm okay with it since I think it's redundant with the two tests (test_fsa_I and test_compute_everything).


@pytest.mark.unit
@pytest.mark.mpl_image_compare(remove_text=True, tolerance=tol_1d)
def test_1d_dpdr():
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with test_1d_p

@pytest.mark.unit
@pytest.mark.solve
@pytest.mark.mpl_image_compare(remove_text=True, tolerance=tol_2d)
def test_2d_lambda(DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with other plot_2d tests

@pytest.mark.unit
@pytest.mark.solve
@pytest.mark.mpl_image_compare(remove_text=True, tolerance=24)
def test_section_Z(DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with plot_section_F

@pytest.mark.unit
@pytest.mark.solve
@pytest.mark.mpl_image_compare(remove_text=True, tolerance=tol_2d)
def test_section_R(DSHAPE_current):
Copy link
Member Author

Choose a reason for hiding this comment

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

redundant with plot_section_F

Copy link
Member Author

Choose a reason for hiding this comment

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

git struggles with the diff here. Mostly was just re-organizing things into classes for testing each of the different functions. I left specific comments on the tests that were actually removed.

@f0uriest f0uriest requested review from dpanici and unalmis April 2, 2024 14:15
dpanici
dpanici previously approved these changes Apr 2, 2024
unalmis
unalmis previously requested changes Apr 3, 2024
Copy link
Collaborator

@unalmis unalmis left a comment

Choose a reason for hiding this comment

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

I think of the two redundant 1d plotting tests, the wrong one was removed (#930 (comment))

tests/test_constrain_current.py Show resolved Hide resolved
unalmis
unalmis previously approved these changes Apr 3, 2024
@f0uriest f0uriest dismissed stale reviews from unalmis and dpanici via 56a7c4c April 4, 2024 00:34
@f0uriest f0uriest merged commit 6acece2 into master Apr 4, 2024
18 checks passed
@f0uriest f0uriest deleted the rc/tests branch April 4, 2024 02:19
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.

4 participants