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

Simplify Named RAI name conversion #323

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Simplify Named RAI name conversion #323

wants to merge 2 commits into from

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Mar 31, 2025

When you use ij.py.from_java, you open the door to unwanted conversion behavior. As an example, if jobj is linked to an ImagePlus, the conversion of the name can utilize imagej-legacy's StringToImagePlusConverter, making the resulting xarray.name an ndarray.

This was the source of imagej/napari-imagej#304

@elevans can you think of any worthwhile tests to add here?

When you use ij.py.from_java, you open the door to unwanted conversion
behavior. As an example, if jobj is linked to an ImagePlus, the
conversion of the name can utilize imagej-legacy's
StringToImagePlusConverter, making the resulting xarray.name an ndarray.

This was the source of imagej/napari-imagej#304
@gselzer gselzer added the bug Something isn't working label Mar 31, 2025
@gselzer gselzer requested a review from elevans March 31, 2025 22:11
@gselzer gselzer self-assigned this Mar 31, 2025
@elevans
Copy link
Member

elevans commented Mar 31, 2025

I wrote minimal example to test this in imagej/napari-imagej#304. We just need to show the image before trying to convert it with ij.py.from_java(). I pulled your branch and your fix works nicely 👍 .

@elevans
Copy link
Member

elevans commented Apr 7, 2025

I haven't really decided on a good test for this and I'm hesitant to add in something like xvfb into our CI jobs. Its an extra layer of complexity for a single test case (and no one has complained about this bug yet). What do you think about just testing to see the xarr.name attribute's type after conversion? It should always be str.

@gselzer
Copy link
Contributor Author

gselzer commented Apr 7, 2025

and no one has complained about this bug yet

So what am I, chopped liver?

What do you think about just testing to see the xarr.name attribute's type after conversion? It should always be str.

Sounds good!

@elevans
Copy link
Member

elevans commented Apr 7, 2025

Doh! Yeah I take that back! Haha the ☕ is still getting into my system.

@gselzer
Copy link
Contributor Author

gselzer commented Apr 7, 2025

I'll add the test momentarily

There really isn't a ton of good places to test this...but this is
better than nothing?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants