No-cache fast painting #6607

Czaki · 2024-01-21T21:41:10Z

References and relevant issues

closes #6579
supersedes #6583

Description

#5732 introduced a cache of mapped data so that only changed indices were mapped to texture dtypes/values and sent on to the GPU. In this PR, an alternate strategy is introduced: rather than caching previously-transformed data and then doing a diff with the cache, we paint the data and the texture-mapped data directly.

The partial update of the on-GPU texture also introduced in #5732 is maintained, as it can dramatically reduce the amount of data needing to be transferred from CPU to GPU memory.

This PR is built on top of #6602.

…(u)int16

jni · 2024-01-22T06:04:11Z

napari/layers/labels/_tests/test_labels.py

@@ -1501,20 +1513,6 @@ def test_invalidate_cache_when_change_color_mode():
    )


-@pytest.mark.parametrize("dtype", np.sctypes['int'] + np.sctypes['uint'])


wtf I only just now noticed np.sctypes exists 😂

jni

I like this! Lots of cache logic removed which is a huge win. 😍

I'm unclear about a lot of parts though so I've made some requests for comments/docstrings.

jni · 2024-01-22T06:21:49Z

napari/layers/labels/labels.py

+    def _get_pt_not_disp(self, indices, values):
+        slice_input = self._slice.slice_input
+        point = np.round(
+            self.world_to_data(slice_input.world_slice.point)
+        ).astype(int)
+        return {dim: point[dim] for dim in slice_input.not_displayed}


This is weird... indices and values aren't used? Can you add a docstring to help me understand why the function signature is what it is?

Yes. I'm sorry. It is an artifact of simplification of code. First version was using it.

jni · 2024-01-22T06:23:56Z

napari/utils/indexing.py

+def visible_items_in_slice(
+    index: Tuple[npt.NDArray[np.int_], ...], position_in_axes: Dict[int, int]
+) -> npt.NDArray[np.bool_]:
+    queries = [
+        index[ax] == position for ax, position in position_in_axes.items()
+    ]
+    return np.logical_and.reduce(queries, axis=0)


Can you add a docstring here? I don't understand what this function is supposed to do. (And I'm not sure it should be public...?)

Added. Should I make it private?

The question is why the whole module is public.

Sure, but given that it is, let's not expand it.

jni · 2024-01-22T06:28:10Z

napari/layers/labels/labels.py

-            displayed_indices = index_in_slice(indices, pt_not_disp)
-            self._slice.image.raw[displayed_indices] = value
+            self._slice.image.raw[displayed_indices] = visible_values
+            # self._slice.image.view[displayed_indices] = self.colormap._map_to_gpu(value)


Remove the comment? It might be worth adding a comment mentioning that the view must also be updated but is done below only if contour == 0 (why?)

It is done only if contour == 0 because in other situation we need to recalculate contours, so use _raw_to_displayed method

jni · 2024-01-22T06:29:42Z

napari/layers/labels/_tests/test_labels.py

@@ -664,6 +664,18 @@ def test_contour_local_updates():
    )


+def test_data_setitem_multi_dim():


Can you describe the bug you found in a docstring here? I still don't understand it after reading the code...

added. Could you check?

Yes, it's great!

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

napari/layers/labels/labels.py

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

napari/utils/_indexing.py

jni

This is nice and small now! 😍 @Czaki can you (a) check my proposed name change (optional) and (b) add my proposed docstring (not optional 😂), then I will merge. 🚀

napari/layers/labels/labels.py

napari/utils/_indexing.py

napari/layers/labels/labels.py

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

for more information, see https://pre-commit.ci

Czaki · 2024-01-22T12:50:36Z

This is nice and small now! 😍 @Czaki can you (a) check my proposed name change (optional) and (b) add my proposed docstring (not optional 😂), then I will merge. 🚀

Done. Let's wait on CI

jni · 2024-01-22T12:57:39Z

@Czaki fyi here is my latest script for manual testing of painting fps:

import sys
import numpy as np
from skimage import data
import napari
import toolz as tz


image = np.tile(data.eagle(), (4, 4))  # about 8k x 8k
labels = np.zeros_like(image, dtype=sys.argv[1] if len(sys.argv) > 1 else int)

v = napari.Viewer()
v.add_image(image)
labels_layer = v.add_labels(labels)
labels_layer.brush_size = 40
labels_layer.mode = 'paint'


@tz.curry
def update_fps(v, fps):
    """Update fps."""
    v.text_overlay.text = f"{fps:1.1f} FPS"

v.text_overlay.visible = True
# note: this is using a private attribute, so it might break
# without warningin future versions!
v.window._qt_viewer.canvas._scene_canvas.measure_fps(callback=update_fps(v))

napari.run()

For uint64 I get about 50-55fps both on this PR and on e0c0c8e... And on main. 😂 I'd be curious if you can test on a normal Windows workstation... (I am in the process of setting up my Linux tower again but it's not running yet.)

codecov · 2024-01-22T13:38:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0f85200) 92.27% compared to head (9ad565f) 92.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6607      +/-   ##
==========================================
- Coverage   92.27%   92.22%   -0.05%     
==========================================
  Files         603      603              
  Lines       53933    53898      -35     
==========================================
- Hits        49767    49708      -59     
- Misses       4166     4190      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jni · 2024-01-22T13:56:56Z

@Czaki I've updated the PR description to provide a bit more background. Please check it for accuracy, especially this bit:

The partial update of the on-GPU texture also introduced in #5732 is maintained, as it can dramatically reduce the amount of data needing to be transferred from CPU to GPU memory.

I wrote that from memory based on the deleted code, I haven't actually verified that it's true. 😅

I am going to bed now, but please merge if my description is accurate, and if you could then incorporate this into #6542 I would be very grateful! 🙏

@psobolewskiPhD if you could re-review #6542 and approve (😬), we can merge and release rc4! 🚀

Goodnight all, looking forward to the morning! 😊

Czaki · 2024-01-22T14:00:38Z

I wrote that from memory based on the deleted code, I haven't actually verified that it's true. 😅

It is true. It still sends only a rectangle containing updated data.

Czaki · 2024-01-22T17:09:12Z

45-60 fps on windows.
(low fps is when do long mouse moves)

closes #6579 supersedes #6583 were mapped to texture dtypes/values and sent on to the GPU. In this PR, an alternate strategy is introduced: rather than caching previously-transformed data and then doing a diff with the cache, we paint the data *and* the texture-mapped data directly. The partial update of the on-GPU texture also introduced in #5732 is maintained, as it can dramatically reduce the amount of data needing to be transferred from CPU to GPU memory. This PR is built on top of #6602. --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

…ccount (#6616) # References and relevant issues closes #6615 # Description In #6607, we started painting into both the data and the slice view. (And this was already happening and broken when painting into lazy arrays such as zarr and dask, #6112.) However, when setting the view we need to take into account the axis ordering used when slicing (some axes may be transposed). This manifested as an index error in #6615. In this PR, we add an `indices_order` argument to `index_in_slice` function and the function returns indices in the requested order, which we can then use to set the view.

…ccount (#6616) closes #6615 In #6607, we started painting into both the data and the slice view. (And this was already happening and broken when painting into lazy arrays such as zarr and dask, #6112.) However, when setting the view we need to take into account the axis ordering used when slicing (some axes may be transposed). This manifested as an index error in #6615. In this PR, we add an `indices_order` argument to `index_in_slice` function and the function returns indices in the requested order, which we can then use to set the view.

Czaki added 4 commits January 19, 2024 10:08

add _map_to_gpu to Labels colormap and remove caching of (u)int8 and …

9882ab6

…(u)int16

add numba cache

4570d10

fix bug in Labels.data_setitem for setting value

2841977

remove cache

5beadc5

github-actions bot added tests Something related to our tests qt Relates to qt labels Jan 21, 2024

Czaki mentioned this pull request Jan 21, 2024

Reduce performance regression from on-CPU mapping of labels before rendering #6583

Closed

efective mapping single values

b6f7ec2

Czaki changed the title ~~add _map_to_gpu to Labels colormap and remove caching of (u)int8 and …~~ No cache fast painting Jan 21, 2024

jni reviewed Jan 22, 2024

View reviewed changes

jni requested changes Jan 22, 2024

View reviewed changes

Czaki and others added 8 commits January 22, 2024 09:05

Update napari/utils/colormaps/colormap.py

80339c3

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

add missed return

fa19858

rename _map_to_gpu to _data_to_texture_dtype

2643124

Merge branch 'do_not_cache_low_bytes' into paint_without_cache

2a20256

fixes after merge

f941d5d

fix tests

bd429e4

rename _data_to_texture_dtype to _data_to_texture

b2ab139

Merge branch 'do_not_cache_low_bytes' into paint_without_cache

9820e4a

Czaki added this to the 0.4.19 milestone Jan 22, 2024

Czaki added the maintenance PR with maintance changes, label Jan 22, 2024

Czaki added 4 commits January 22, 2024 12:54

fix method name

f1ceed8

add docstrings

9154c0d

fix test and add docstring

e3b8a0d

Merge branch 'main' into paint_without_cache

212f08b

jni reviewed Jan 22, 2024

View reviewed changes

napari/layers/labels/labels.py Outdated Show resolved Hide resolved

Czaki and others added 2 commits January 22, 2024 13:29

make indexing private

2c7654f

Update napari/layers/labels/labels.py

6bd9eb8

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

jni reviewed Jan 22, 2024

View reviewed changes

napari/utils/_indexing.py Outdated Show resolved Hide resolved

jni approved these changes Jan 22, 2024

View reviewed changes

Czaki commented Jan 22, 2024

View reviewed changes

napari/layers/labels/labels.py Outdated Show resolved Hide resolved

napari/utils/_indexing.py Outdated Show resolved Hide resolved

napari/layers/labels/labels.py Outdated Show resolved Hide resolved

Czaki and others added 2 commits January 22, 2024 13:49

Apply suggestions from code review

c825903

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

96b889f

for more information, see https://pre-commit.ci

jni added the ready to merge Last chance for comments! Will be merged in ~24h label Jan 22, 2024

jni changed the title ~~No cache fast painting~~ No-cache fast painting Jan 22, 2024

remove obsolete method, ignore napari/utils/indexing.py in coverage

9ad565f

Czaki merged commit ded3311 into napari:main Jan 22, 2024
32 checks passed

Czaki deleted the paint_without_cache branch January 22, 2024 17:09

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Jan 22, 2024

psobolewskiPhD mentioned this pull request Jan 23, 2024

Setting color parameter in add_labels causes long execution time #3852

Open

jni mentioned this pull request Jan 24, 2024

Fix Labels.data_setitem setting of view by taking dims order into account #6616

Merged

psobolewskiPhD mentioned this pull request Jan 26, 2024

Allow use functions from PartSegCore-compiled-backend as numba alternative for data to texture mapping #6617

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No-cache fast painting #6607

No-cache fast painting #6607

Czaki commented Jan 21, 2024 •

edited by jni

jni Jan 22, 2024

jni left a comment

jni Jan 22, 2024

Czaki Jan 22, 2024

jni Jan 22, 2024

Czaki Jan 22, 2024

jni Jan 22, 2024

Czaki Jan 22, 2024

jni Jan 22, 2024

Czaki Jan 22, 2024

jni Jan 22, 2024

Czaki Jan 22, 2024

jni Jan 22, 2024

jni left a comment

Czaki commented Jan 22, 2024

jni commented Jan 22, 2024

codecov bot commented Jan 22, 2024 •

edited

jni commented Jan 22, 2024

Czaki commented Jan 22, 2024

Czaki commented Jan 22, 2024

		@@ -1501,20 +1513,6 @@ def test_invalidate_cache_when_change_color_mode():
		)


		@pytest.mark.parametrize("dtype", np.sctypes['int'] + np.sctypes['uint'])

		@@ -664,6 +664,18 @@ def test_contour_local_updates():
		)


		def test_data_setitem_multi_dim():

No-cache fast painting #6607

No-cache fast painting #6607

Conversation

Czaki commented Jan 21, 2024 • edited by jni

References and relevant issues

Description

Choose a reason for hiding this comment

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni left a comment

Choose a reason for hiding this comment

Czaki commented Jan 22, 2024

jni commented Jan 22, 2024

codecov bot commented Jan 22, 2024 • edited

Codecov Report

jni commented Jan 22, 2024

Czaki commented Jan 22, 2024

Czaki commented Jan 22, 2024

Czaki commented Jan 21, 2024 •

edited by jni

codecov bot commented Jan 22, 2024 •

edited