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

Return early when no slicing needed #5239

Merged
merged 1 commit into from Oct 19, 2022

Conversation

andy-sweet
Copy link
Member

Description

This fixes the slicing performance issues detected in #5194. These were caused by the refactor in #5003, which removed the case where we return early in Layer._slice_indices when no slicing is actually needed.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

References

Closes #5194

How has this been tested?

  • all existing tests pass with my change

I ran some of the slicing benchmarks affected by this in #5194 at a few different commits.

First, at the commit on main corresponding to #5003 (2566980).

(napari-dev) ➜  napari git:(256698021) asv run --bench "Image2DSuite.time_set_view_slice" --python=same
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_asweet_software_miniconda3_envs_napari-dev_bin_python
[ 25.00%] ··· Running (benchmark_image_layer.Image2DSuite.time_set_view_slice--)..
[ 75.00%] ··· benchmark_image_layer.Image2DSuite.time_set_view_slice                                                                                                                                                                                                      ok
[ 75.00%] ··· ======== ==========
               param1            
              -------- ----------
                 16     717±20μs 
                 32     714±10μs 
                 64     752±10μs 
                128     755±70μs 
                256     768±40μs 
                512     736±9μs  
                1024    743±10μs 
                2048    762±30μs 
                4096    720±30μs 
              ======== ==========

[100.00%] ··· benchmark_qt_slicing.AsyncImage2DSuite.time_set_view_slice                                                                                                                                                                                                  ok
[100.00%] ··· ======== ============ ======================
              --                      param2              
              -------- -----------------------------------
               param1   skin_data    jrc_hela-2 (scale 3) 
              ======== ============ ======================
                0.0     1.49±0.1ms        4.78±0.1ms      
                0.05     319±2ms           5.01±0s        
                0.1      616±2ms           9.78±0s        
              ======== ============ ======================

Then at the previous commit (e6ab7cd) to show the regression.

(napari-dev) ➜  napari git:(e6ab7cda8) asv run --bench "Image2DSuite.time_set_view_slice" --python=same
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_asweet_software_miniconda3_envs_napari-dev_bin_python
[ 25.00%] ··· Running (benchmark_image_layer.Image2DSuite.time_set_view_slice--)..
[ 75.00%] ··· benchmark_image_layer.Image2DSuite.time_set_view_slice                                                                                                                                                                                                      ok
[ 75.00%] ··· ======== ===========
               param1             
              -------- -----------
                 16     92.1±30μs 
                 32      66.2±2μs 
                 64      66.3±3μs 
                128      64.3±5μs 
                256      66.3±2μs 
                512      65.3±2μs 
                1024     66.1±4μs 
                2048     72.0±6μs 
                4096     66.2±3μs 
              ======== ===========

[100.00%] ··· benchmark_qt_slicing.AsyncImage2DSuite.time_set_view_slice                                                                                                                                                                                                  ok
[100.00%] ··· ======== =========== ======================
              --                     param2              
              -------- ----------------------------------
               param1   skin_data   jrc_hela-2 (scale 3) 
              ======== =========== ======================
                0.0      736±60μs        4.85±0.3ms      
                0.05    320±0.9ms        5.06±0.01s      
                0.1     614±0.7ms        9.80±0.02s      
              ======== =========== ======================

Then with this PR, to show the fix.

(napari-dev) ➜  napari git:(fix-5194-early-return-slice) asv run --bench "Image2DSuite.time_set_view_slice" --python=same
· Discovering benchmarks
· Running 2 total benchmarks (1 commits * 1 environments * 2 benchmarks)
[  0.00%] ·· Benchmarking existing-py_Users_asweet_software_miniconda3_envs_napari-dev_bin_python
[ 25.00%] ··· Running (benchmark_image_layer.Image2DSuite.time_set_view_slice--)..
[ 75.00%] ··· benchmark_image_layer.Image2DSuite.time_set_view_slice                                                                                                                                                                                                        ok
[ 75.00%] ··· ======== ============
               param1              
              -------- ------------
                 16      68.9±9μs  
                 32     67.6±0.7μs 
                 64      69.3±6μs  
                128      64.5±2μs  
                256      67.8±1μs  
                512      64.8±1μs  
                1024     67.7±2μs  
                2048     67.5±2μs  
                4096     70.3±3μs  
              ======== ============

[100.00%] ··· benchmark_qt_slicing.AsyncImage2DSuite.time_set_view_slice                                                                                                                                                                                                    ok
[100.00%] ··· ======== =========== ======================
              --                     param2              
              -------- ----------------------------------
               param1   skin_data   jrc_hela-2 (scale 3) 
              ======== =========== ======================
                0.0      700±8μs         5.15±0.8ms      
                0.05     320±1ms          5.03±0s        
                0.1     614±0.8ms        9.85±0.01s      
              ======== =========== ======================

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #5239 (13efd57) into main (fb81ff6) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5239      +/-   ##
==========================================
+ Coverage   88.74%   88.79%   +0.04%     
==========================================
  Files         574      574              
  Lines       48954    48956       +2     
==========================================
+ Hits        43446    43472      +26     
+ Misses       5508     5484      -24     
Impacted Files Coverage Δ
napari/layers/base/base.py 91.49% <100.00%> (+0.02%) ⬆️
napari/components/experimental/chunk/_pool.py 85.71% <0.00%> (-7.94%) ⬇️
napari/_qt/qt_main_window.py 75.41% <0.00%> (+0.18%) ⬆️
napari/layers/image/image.py 95.89% <0.00%> (+0.24%) ⬆️
napari/utils/theme.py 89.79% <0.00%> (+0.68%) ⬆️
napari/_qt/qt_event_loop.py 82.31% <0.00%> (+0.68%) ⬆️
napari/utils/interactions.py 74.41% <0.00%> (+2.32%) ⬆️
napari/layers/image/_image_slice.py 100.00% <0.00%> (+2.38%) ⬆️
napari/_qt/widgets/qt_color_swatch.py 74.79% <0.00%> (+3.25%) ⬆️
napari/utils/info.py 83.33% <0.00%> (+4.44%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andy-sweet
Copy link
Member Author

Thanks for the reviews @jni and @Czaki. I will merge this after 24 hours.

@kcpevey
Copy link
Contributor

kcpevey commented Oct 19, 2022

Nice fix! :)

@andy-sweet andy-sweet merged commit c0fee1a into napari:main Oct 19, 2022
@andy-sweet andy-sweet deleted the fix-5194-early-return-slice branch October 19, 2022 14:21
@andy-sweet andy-sweet added bug Something isn't working performance Relates to performance labels Oct 19, 2022
@andy-sweet andy-sweet added this to the 0.5.0 milestone Dec 8, 2022
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 18, 2023
@Czaki Czaki added bugfix PR with bugfix and removed bug Something isn't working labels Jun 18, 2023
Czaki pushed a commit that referenced this pull request Jun 18, 2023
@Czaki Czaki mentioned this pull request Jun 18, 2023
Czaki pushed a commit that referenced this pull request Jun 19, 2023
Czaki pushed a commit that referenced this pull request Jun 21, 2023
Czaki pushed a commit that referenced this pull request Jun 21, 2023
Czaki pushed a commit that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix performance Relates to performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[test-bot] Benchmark tests failing
4 participants