Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: rgb, v2 #141

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

feat: rgb, v2 #141

wants to merge 17 commits into from

Conversation

gselzer
Copy link
Collaborator

@gselzer gselzer commented Feb 20, 2025

This PR revives the work done in #41, but updated for the new viewer. I'm guessing that the design can be further refined, and there are more bugs to be ironed out.

image

Still lots of cleaning, testing, bugfixing to do. Notably, there were many comments on #41 that should be gone through and addressed here.

Note that histograms are broken :)
@gselzer gselzer added the enhancement New feature or request label Feb 20, 2025
@gselzer gselzer self-assigned this Feb 20, 2025
@gselzer gselzer mentioned this pull request Feb 20, 2025
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 84.07080% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.34%. Comparing base (0f7b61c) to head (235e21a).

Files with missing lines Patch % Lines
src/ndv/controllers/_array_viewer.py 66.66% 6 Missing ⚠️
src/ndv/views/_jupyter/_array_view.py 44.44% 5 Missing ⚠️
src/ndv/views/_wx/_array_view.py 68.75% 5 Missing ⚠️
src/ndv/models/_data_display_model.py 96.66% 1 Missing ⚠️
src/ndv/views/_pygfx/_array_canvas.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   80.35%   80.34%   -0.01%     
==========================================
  Files          44       44              
  Lines        4112     4178      +66     
==========================================
+ Hits         3304     3357      +53     
- Misses        808      821      +13     

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

@gselzer gselzer mentioned this pull request Feb 20, 2025
src/ndv/data.py Outdated
Comment on lines 150 to 160
def rgba() -> np.ndarray:
img = np.zeros((4, 256, 256), dtype=np.uint8)

for x in range(256):
for y in range(256):
img[0, x, y] = x
img[1, x, y] = y
img[2, x, y] = 255 - x
img[3, x, y] = int(sqrt((x - 128) ** 2 + (y - 128) ** 2))

return img
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlambert03 I copied this over from #41, but do we want/need it if we have astronaut as well? Which should the example use?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy only using astronaut, but if you find this one useful for debugging, then I don't mind leaving it in (but add to the docstring when someone might want to use it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, well I just wrote a commit deleting it, however I'm now realizing that there's no existing dataset with an alpha channel...so I added it back in 3761b9b, but augmented it into a 3D RGB dataset (with a short docstring).

It's useful for testing higher-dim RGB data, as might come from a RGB
camera MDA via Micro-Manager in the real world
@gselzer
Copy link
Collaborator Author

gselzer commented Feb 21, 2025

@tlambert03 I'm generally happy with the functionality here and would appreciate your thoughts and high-level review. Couple points I'd like your opinions on:

  • I've changed the LutKey type alias into ChannelKey, which now is basically anything Hashable. The reason that I need this is because I need a specific identifier for RGB mode - it cannot be an integer, and must be differentiable from None - to trigger an appropriate RGB LutView and to correctly slice (read: not slice) the data. However this typing may be too broad - what do you think?
  • On a broader level, I'm not quite satisfied with how much RGB mode gets special-cased - in a sense, it needs special casing because it's the only mode we (want to) support where the channels must be combined for meaning - but it would be nice to minimize the special cases.
  • The histogram right now is lacking because there's no special RGB support - I'm thinking this may best be solved in another PR, because it's not technically wrong right now.
  • There's an obnoxious case where, if you do not specify RGB mode on viewer construction (i.e. remove the data model from being passed to imshow in the new example, you'll start out in composite mode where the channel is one of the visible axes. I try to make switching to RGB do the right thing and change the visible axes (in _ArrayDataDisplayModel._on_channel_mode_change), but then there's no way to go back to the previously shown axes. This feels unfortunate/wrong (hence the leftover FIXME), but maybe it's just necessary?
  • This could just be random, but I feel like recent commits have more tests that sporadically fail. I didn't edit any tests so I'm not sure what would be causing this - if you have any ideas I'd be interested!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants