Add _data_to_texture method to LabelColormap and remove caching of (u)int8 and (uint16) #6602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Czaki
force-pushed
the
do_not_cache_low_bytes
branch
from
fd7cc93
to
9882ab6
Compare
Czaki
added
the
run-benchmarks
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6602 +/- ##
==========================================
- Coverage 92.27% 92.23% -0.05%
==========================================
Files 603 603
Lines 53902 53933 +31
==========================================
+ Hits 49737 49743 +6
- Misses 4165 4190 +25 ☔ View full report in Codecov by Sentry. |
This was referenced Jan 21, 2024
Merged
jni
approved these changes
Jan 22, 2024
napari/utils/colormaps/colormap.py
Outdated
Comment on lines
287
to
298
| @overload | ||
| def _map_to_gpu(self, values: np.ndarray) -> np.ndarray: | ||
| ... | ||
|
|
||
| @overload | ||
| def _map_to_gpu(self, values: np.integer) -> np.integer: | ||
| ... | ||
|
|
||
| def _map_to_gpu( | ||
| self, values: Union[np.ndarray, np.integer] | ||
| ) -> Union[np.ndarray, np.integer]: | ||
| """Map input values to values for send to GPU.""" |
There was a problem hiding this comment.
Suggested change
| @overload | |
| def _map_to_gpu(self, values: np.ndarray) -> np.ndarray: | |
| ... | |
| @overload | |
| def _map_to_gpu(self, values: np.integer) -> np.integer: | |
| ... | |
| def _map_to_gpu( | |
| self, values: Union[np.ndarray, np.integer] | |
| ) -> Union[np.ndarray, np.integer]: | |
| """Map input values to values for send to GPU.""" | |
| @overload | |
| def _data_to_texture_dtype(self, values: np.ndarray) -> np.ndarray: | |
| ... | |
| @overload | |
| def _data_to_texture_dtype(self, values: np.integer) -> np.integer: | |
| ... | |
| def _data_to_texture_dtype( | |
| self, values: Union[np.ndarray, np.integer] | |
| ) -> Union[np.ndarray, np.integer]: | |
| """Map input values to a (possibly reduced) dtype to set a GPU texture.""" |
... Typing question: since the overload is defined in the base class, is it also needed here?
There was a problem hiding this comment.
mypy was complaining about lack of overload...
napari/utils/colormaps/colormap.py
Outdated
Comment on lines
377
to
379
| @overload | ||
| def _map_to_gpu(self, values: np.ndarray) -> np.ndarray: | ||
| ... |
There was a problem hiding this comment.
Suggested change
| @overload | |
| def _map_to_gpu(self, values: np.ndarray) -> np.ndarray: | |
| ... | |
| @overload | |
| def _data_to_texture_dtype(self, values: np.ndarray) -> np.ndarray: | |
| ... |
| @@ -887,7 +887,7 @@ def test_background_color(qtbot, qt_viewer: QtViewer, dtype): | |||
| data = np.zeros((10, 10), dtype=dtype) | |||
| data[5:] = 10 | |||
| layer = qt_viewer.viewer.add_labels(data, opacity=1) | |||
| color = layer.colormap.map(10)[0] * 255 | |||
| color = layer.colormap.map(10) * 255 | |||
There was a problem hiding this comment.
As you know, I like this change, but when I tried to do it you told me that it might be a breaking change for some users... I don't mind going in this direction either way — it is an easy fix for downstream users and matches the documentation.
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
|
@jni Ready to merge? |
jni
changed the title
|
Done! 🚀 |
Czaki
added a commit
that referenced
this pull request
Jan 22, 2024
# 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. --------- 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>
Czaki
added a commit
that referenced
this pull request
Jan 23, 2024
…)int8 and (uint16) (#6602) # Description In this PR, I extract the most obvious parts of #6583 to allow cherrypick them to 0.4.19. The changes are: 1) As we use memory view for (u)int8 and (u)int16 data, this PR removes caching in `_raw_to_dispayed` as it is obsolete. 2) add a `_data_to_texture` method to `LabelColormap` to provide uniform interface for mapping between the data dtype and the (possibly smaller, unsigned) texture dtype and avoid branching. 3) fix `Colormap.map` for `int` to fit documentation. --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Czaki
added a commit
that referenced
this pull request
Jan 23, 2024
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In this PR, I extract the most obvious parts of #6583 to allow cherrypick them to 0.4.19.
The changes are:
_raw_to_dispayedas it is obsolete._data_to_texturemethod toLabelColormapto provide uniform interface for mapping between the data dtype and the (possibly smaller, unsigned) texture dtype and avoid branching.Colormap.mapforintto fit documentation.