Skip to content

Simplify Named RAI name conversion #323

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.

Sign up for GitHub

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

Jump to bottom
Merged
elevans merged 2 commits into from
Apr 7, 2025
Merged

Simplify Named RAI name conversion #323

elevans merged 2 commits into from
Apr 7, 2025

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?

@gselzer
Simplify Named RAI name conversion
b3ccc86 
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 •
edited
Loading

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

@gselzer
Add a small test on Java->Python name conversion
778d253 
There really isn't a ton of good places to test this...but this is
better than nothing?
elevans
elevans approved these changes Apr 7, 2025
View reviewed changes
Copy link
Member

@elevans elevans left a comment

Choose a reason for hiding this comment

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

Looks great!

@elevans elevans merged commit d066ae7 into main Apr 7, 2025
8 checks passed
@elevans elevans deleted the rai-name branch April 7, 2025 16:14
@gselzer gselzer mentioned this pull request Apr 7, 2025
Draft
@imagesc-bot
Copy link

imagesc-bot commented Apr 10, 2025

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/fiji-friends-weekly-dev-update-thread/103718/85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elevans elevans elevans approved these changes

Assignees

@gselzer gselzer

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gselzer @elevans @imagesc-bot