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

Fix invalid Shape.TheC value #5

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

melissalinkert
Copy link
Member

Noticed while testing glencoesoftware/ome-omero-roitool#38:

  • open any OMERO image in QuPath using this extension
  • draw a simple shape
  • send annotations back to OMERO
  • try to export the annotation with ome-omero-roitool, get an exception because Shape.TheC is -1

ome-omero-roitool will get a companion PR shortly, but this should at least prevent the extension from sending an invalid value. Repeating the same steps as above with this change and a different image should successfully export the ROI to OME-XML.

The* values should always be non-negative, -1 is invalid.
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Confirmed as part of the testing of glencoesoftware/ome-omero-roitool#38 that the current release of the QuPath OMERO extension sets theC to -1 for each Shape exported to OMERO. With this PR included, theC is set to 0 and the ROIs can be exported as OME-XML using the current HEAD of ome-omero-roitool.

Is setting this value unconditionally to the first channel the expected beahvior? An alternative default would be not to set TheC which means the ROI would be applied to all channels.

int will always be serialized, but Integer will only be serialized if not null.
@melissalinkert
Copy link
Member Author

With defb154, The* should not be set when exporting from QuPath to OMERO. Using Integer instead of int means the values will only be serialized to JSON if not null. Since nothing in the ROI export logic changes the plane indexes, these values should always be null and thus no longer set in OMERO. ome-omero-roitool should be able to export ROIs without The* set.

@sbesson sbesson self-requested a review May 30, 2024 20:15
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

With the last commit, using a multi-channel multi-Z fluorescence images hosted on OMERO and exporting annotations created from QuPath, I get the following behavior

Screenshot 2024-05-31 at 09 35 20 Screenshot 2024-05-31 at 09 35 14

All theC, theZ and theT properties are now unset and the ROIs will be interpreted as applying to all planes.

A possible follow-up would be to look into populating the TheZ value in QuPath and translate it accordingly when exporting to OMERO. My understanding is that the Z/T index was not properly populated prior to this PR so this is not a regression. It would be useful to have a second opinion from @muhanadz or anyone involved in the QuPath workflow.

@sbesson sbesson merged commit d4c0064 into glencoesoftware:main Jun 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants