napari / napari

napari: a fast, interactive, multi-dimensional image viewer for python
https://napari.org
BSD 3-Clause "New" or "Revised" License
2.1k stars 410 forks source link

perf: remove extra highlight calls #6893

Closed DanGonite57 closed 1 month ago

DanGonite57 commented 1 month ago

References and relevant issues

Contributes towards https://github.com/napari/napari/issues/6746

Description

The Points.selected_data setter calls _set_highlight itself before returning. In the two bulk-selection/deselection keybindings, _set_highlight is also called, essentially immediately after the setter, resulting in duplication of work and non-negligible overhead particularly with large numbers of points.

bench.py

import numpy as np

from napari.layers.points import Points, _points_key_bindings as key_bindings

rng = np.random.default_rng(seed=0)
data = rng.random(size=(1_000_000, 2)) * 1000
layer = Points(data, size=1)
layer.mode = 'select'
layer._set_view_slice()

key_bindings.select_all_in_slice(layer)

Before:

> hyperfine --warmup 1 .\bench.py
Benchmark 1: .\bench.py
  Time (mean ± σ):     10.102 s ±  0.209 s    [User: 7.505 s, System: 0.640 s]
  Range (min … max):    9.888 s … 10.586 s    10 runs

After:

> hyperfine --warmup 1 .\bench.py
Benchmark 1: .\bench.py
  Time (mean ± σ):      8.936 s ±  0.129 s    [User: 6.152 s, System: 0.668 s]
  Range (min … max):    8.745 s …  9.176 s    10 runs

This change may clash with previous fix https://github.com/napari/napari/pull/5771. However, I don't seem to have been able to visually reproduce any difference between _set_highlight(), _set_highlight(force=true), or this removal under the conditions highlighted in the issue, so something may have changed somewhere else since then. From my brief understanding, at present using the keybinds will always change the set of selected points, which means that highlights will always be re-rendered regardless of force (unless there are no points, in which case there will be no points to change the selection/highlighting of either).

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.43%. Comparing base (1dcbade) to head (30e10e4). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6893 +/- ## ========================================== - Coverage 92.45% 92.43% -0.02% ========================================== Files 617 617 Lines 55166 55164 -2 ========================================== - Hits 51001 50992 -9 - Misses 4165 4172 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

psobolewskiPhD commented 1 month ago

Yup, can't reproduce the issue with this PR, so seems good.