Fix direct colormap

[Czaki]

Description

This fixes a bug on the current main implementation of direct color mode in the Labels layer that is required for #6439 grade.

[Czaki]

@jni, this is a real fix.

As without this change the _DTYPE_TO_VISPY_FORMAT[np.float32] return None, not "r32f" here:

napari/napari/_vispy/layers/labels.py

Line 581 in 8ed7f4c

self._setup_nodes(_DTYPE_TO_VISPY_FORMAT[dtype])

[brisvag]

please add a comment explaining this just above, it seems easy to overlook this in the future and move it back to the dict declaration.

[Czaki]

Ok now?

[Czaki]

this only silence openGL warning

[jni]

@Czaki either here or in #6439 we should figure out why tests didn't catch this...

[Czaki]

@jni Test does not catch this because we do not test it :P. The #6439 contains one test for this.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (78900b0) 92.21% compared to head (a8383b7) 92.16%.

Files	Patch %	Lines
napari/_vispy/layers/labels.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6461      +/-   ##
==========================================
- Coverage   92.21%   92.16%   -0.05%     
==========================================
  Files         601      601              
  Lines       53135    53167      +32     
==========================================
+ Hits        48996    49003       +7     
- Misses       4139     4164      +25     

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

[jni]

Test does not catch this because we do not test it :P.

Well, we have some tests, e.g.

napari/napari/layers/labels/_tests/test_labels.py

Lines 1446 to 1454 in ef26ef3

	def test_large_labels_direct_color():
	"""Make sure direct color works with large label ranges"""
	data = np.array([[0, 1], [2**16, 2**20]], dtype=np.uint32)
	colors = {1: 'white', 2**16: 'green', 2**20: 'magenta'}
	layer = Labels(data)
	layer.color = colors
	assert layer.color_mode == 'direct'
	np.testing.assert_allclose(layer.get_color(2**20), [1.0, 0.0, 1.0, 1.0])

but they are bad tests, apparently. 😅 I indeed didn't find any in the vispy or qt tests.

Do you want to bring the test over to this PR @Czaki? I think it would be nice to have it here.

[jni]

@Czaki weird failures on this...

  ________ ERROR at teardown of test_label_colors_matching_widget[False] _________
  
  qtbot = <pytestqt.qtbot.QtBot object at 0x7fe0f7e735d0>
  
      @pytest.fixture()
      def qt_viewer(qtbot):
          qt_viewer = QtViewer(viewer=ViewerModel())
          qtbot.add_widget(qt_viewer)
          qtbot.add_widget(qt_viewer.controls)
          qt_viewer.show()
          qt_viewer.controls.show()
          yield qt_viewer
          qt_viewer.controls.hide()
  >       qt_viewer.hide()
  E       RuntimeError: wrapped C/C++ object of type QtViewer has been deleted
  
  napari/_qt/_tests/test_qt_viewer.py:694: RuntimeError

[Czaki]

@Czaki weird failures on this...

looks fixed

[jni]

Awesome!

@Czaki do you think you can explain in plain terms what the problem was with the Qt tests? I don't really follow the commit. It is probably a good investment to write a developer documentation page that explains the issues with testing Qt, cleanup failures, and how to avoid them...

This should be a later PR. Maybe I'll make an issue?

[Czaki]

@Czaki do you think you can explain in plain terms what the problem was with the Qt tests?

No. I do not understand.

The only difference is to stop using qtbot to manage QtViewers and switch to manually manage. This was to fix leake widget warnings (as make_napari_viewer requires strict cleanup after tet and qtbot do this lazy).

But I do not understand how use qtbot leads to RuntimeError: wrapped C/C++ object of type QtViewer has been deleted

[jni]

Ok. That's a bummer. 😂

Anyway, thinking about reviewing #6459, I realise now that selection + 1 is actually a bug: when the selection has been mapped to n, it will now be outside of the range of values in the texture (rather than looping back to 1), and nothing will be selected. We need to cast using the same function as the whole texture.

[Czaki]

I realise now that selection + 1 is actually a bug

It is an intentional bug... it is part of a bigger problem that I'm working to solve in follow-up PR.
It is an improvement of current state as selection will start working for values below num_colors

We need to cast using the same function as the whole texture.

Nope. This solution will show all components mapping to the same color. not only selected label.