pyapp-kit / ndv

Simple, fast-loading, n-dimensional array viewer with minimal dependencies.
BSD 3-Clause "New" or "Revised" License
40 stars 7 forks source link

feat: RGB support #41

Open gselzer opened 2 months ago

gselzer commented 2 months ago

This PR aims to enable the viewing of RGB datasets. Users specify that their provided data is RGB using the new ChannelMode value. The current mechanism enables this mode when the channel axis (specified through existing means) has exactly three indices.

To enable the selection of different modes, the ChannelModeButton is refactored into a ChannelModeComboBox, which can be altered based on the data to contain the rgb mode.

image

This PR requires review, and likely additional minor cleanup.

Closes #20

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 88.23529% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.87%. Comparing base (10abe13) to head (783dd39).

Files with missing lines Patch % Lines
src/ndv/viewer/_backends/_pygfx.py 61.53% 5 Missing :warning:
src/ndv/viewer/_components.py 80.95% 4 Missing :warning:
src/ndv/viewer/_viewer.py 93.61% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #41 +/- ## ========================================== - Coverage 77.90% 77.87% -0.04% ========================================== Files 13 13 Lines 1856 1885 +29 ========================================== + Hits 1446 1468 +22 - Misses 410 417 +7 ```

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

gselzer commented 2 months ago

Converting this back into a draft because additional changes are needed to support smooth transitions between modes

gselzer commented 2 months ago

I'm much happier with this after a couple of changes, however I've built it on top of #40, so that PR must be merged first

tlambert03 commented 2 months ago

merged #40, can you resolve conflicts?

gselzer commented 2 months ago

merged #40, can you resolve conflicts?

Sure!

gselzer commented 2 months ago

@tlambert03 conflicts resolved!

tlambert03 commented 2 months ago

~could you add that nifty rgb demo to the examples folder so we have a quick way to test rgb?~

how did i miss it 🤦‍♂️ ... sorry :)

gselzer commented 2 months ago

@tlambert03 added a WIP commit fleshing out alpha support. Couple questions:

gselzer commented 2 months ago

Oh, and one more discussion point @tlambert03:

These SETUP lines have given me a bit of a headache. I've found that if you set the channel mode to RGB before setting the data, the channel axis is present as a slider, until you select a different mode (because NDViewer.set_channel_mode contains the logic for removing the channel slider if not mono mode). If you set the channel mode after, we end up in this strange intermediate state where RGB mode is "selected", but I can only see the final channel of the data. Thoughts?

image

tlambert03 commented 2 months ago

Oh, and one more discussion point @tlambert03:

These SETUP lines have given me a bit of a headache.

played around with this a bit... i feel like setting channel mode second "feels" more correct (since it's more likely to be changed in the course of viewing). does this work for you? https://github.com/gselzer/ndv/pull/3

gselzer commented 1 month ago
* Shall we rename the channel mode (e.g. `rgba`?)

I took the liberty of renaming it!

tlambert03 commented 1 month ago

This is all looking good and we can finish it up this week, I was thinking that the main thing to fix that race condition is probably just to remove the conditional check for existing image handles (there's little harm in calling refresh anyway, and it will cancel any waiting futures anyway). So, that, combined with putting the call to set channel mode after set data mode in the init should be good I hope

tlambert03 commented 1 month ago

in e1ae37c I removed the conditional checks before calling self.refresh() ... i'm curious to know whether you can still hit the problem mentioned in https://github.com/pyapp-kit/ndv/pull/41#issuecomment-2356287035

gselzer commented 1 month ago

in e1ae37c I removed the conditional checks before calling self.refresh() ... i'm curious to know whether you can still hit the problem mentioned in #41 (comment)

I cannot see either issue now! No slider, and I'm seeing an rgba image! Great!

tlambert03 commented 1 month ago

seems that:

# run rgb example then...

In [15]: wdg.set_data(ndv.data.cells3d())

In [16]: wdg.set_data(img)

does show img as rgb, but composite is still selected in the combobox.

(sidenote: cells3d is shown as noise ... which is weird, but probably unrelated here)

gselzer commented 1 month ago

@tlambert03 I'd really like to squash all these commits together soon, as the history is rather messy, and do one final review.

This script is helpful for testing:

import math

import numpy

import ndv
from qtpy.QtWidgets import QApplication, QPushButton

# 2D RGB dataset
rgb = numpy.zeros((256, 256, 4), dtype=numpy.uint8)
for x in range(256):
    for y in range(256):
        rgb[x, y, 0] = x
        rgb[x, y, 1] = y
        rgb[x, y, 2] = 255 - x
        rgb[x, y, 3] = int(math.sqrt((x - 128) ** 2 + (y - 128) ** 2))

# 4D ZCYX dataset
cells = ndv.data.cells3d()

# Button to switch between them
def switch(checked: bool) -> None:
    n.set_data(rgb if checked else cells)
app = QApplication([])
btn = QPushButton("Switch image")
btn.setCheckable(True)
btn.toggled.connect(switch)

# Run it!
n = ndv.NDViewer()
n._btns.addWidget(btn)
n.show()
app.exec()