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 doctest of calibrate_denoiser by ~30 s #7209

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lagru
Copy link
Member

@lagru lagru commented Oct 17, 2023

Description

denoise_tv_chambolle (34.01 s) is a lot slower than denoise_wavelet (2.14 s). denoise_wavelet, was previously replaced because PyWavelets got turned into an optional dependency and handling this in doctests is awkward. So let's revert this change.

denoise_tv_bregman would be another faster option (5.23 s) but I am not sure if using something different from denoise_wavelet actually makes sense from an algorithm standpoint.

Other top slowest tests in our suite (use --duration=X with pytest to get X slowest tests):

========================================== slowest durations ==========================================
34.01s call     restoration/j_invariant.py::skimage.restoration.j_invariant.denoise_invariant
32.39s call     data/tests/test_data.py::test_download_all_with_pooch
16.65s call     registration/tests/test_tvl1.py::test_3d_motion
15.12s call     restoration/tests/test_rolling_ball.py::test_ndim
12.85s call     graph/tests/test_rag.py::test_reproducibility
7.16s call     restoration/_rolling_ball.py::skimage.restoration._rolling_ball.rolling_ball
4.00s call     filters/tests/test_ridges.py::test_3d_cropped_camera_image
3.95s call     registration/_optical_flow.py::skimage.registration._optical_flow.optical_flow_tvl1
3.31s call     registration/tests/test_ilk.py::test_3d_motion[True-True]
3.10s call     registration/tests/test_ilk.py::test_3d_motion[True-False]
2.84s call     measure/tests/test_blur_effect.py::test_blur_effect_3d
2.65s call     segmentation/tests/test_random_walker.py::test_spacing_1
2.60s call     morphology/tests/test_footprints.py::test_nsphere_series_approximation[100-ball]
2.58s call     feature/sift.py::skimage.feature.sift.SIFT
...

Checklist

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

...

denoise_tv_chambolle (34.01 s) is a lot slower than denoise_wavelet
(2.14 s). denoise_wavelet, was previously replaced because PyWavelets
got turned into an optional dependency and handling this in doctests
is awkward. So let's revert this change.

denoise_tv_bregman would be another faster option (5.23 s) but I am not
sure if using something different from denoise_wavelet actually makes
sense from an algorithm standpoint.
@lagru lagru added the 🔧 type: Maintenance Refactoring and maintenance of internals label Oct 17, 2023

In this case, `denoise_wavelet` requires the optional dependency PyWavelets.

>>> import pytest; _ = pytest.importorskip("pywt")
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we could hide this line altogether.

@pllim does doctest-plus support something like that? I don't see an directive at a first glance.

Copy link

Choose a reason for hiding this comment

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

Maybe you want .. doctest-requires:: documented in https://github.com/scientific-python/pytest-doctestplus#doctest-dependencies ?

Copy link
Member Author

@lagru lagru Oct 17, 2023

Choose a reason for hiding this comment

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

That seems like a good option and in my book it's an argument in favor of using doctest-plus! Thanks for the quick reply. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants