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

Implement setROIName in OMEROMetadataStoreClient #90

Merged
merged 1 commit into from
Jun 22, 2020

Conversation

melissalinkert
Copy link
Member

Importing an OME-TIFF file that defines an ROI with the Name attribute set currently results in the name being lost. This change should make it so that the ROI name is correctly saved during import.

I'd guess this was just an oversight when names were added in ome/openmicroscopy#3362, but if there is a reason why the name can't be passed through then happy to discuss alternatives.

A minimal pyramid OME-TIFF with one named ROI is in /uod/idr/scratch/inbox/with-roi.ome.tiff.

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.

Changes look good.

Used the sample OME-TIFF generated from MitoCheck and imported it with and without this PR. In both cases, many ROIs with a single rectangle shapes were populated.

Screen Shot 2020-06-08 at 15 15 01

I was surprised to see the same value for Comments in both imports until I realized that OMERO.iviewer displays Shape.Text but not ROI.Name. This feels a bit at odds with the UI display which primarily refers to ROI. Maybe a RFE to include an optional column with the ROI name? /cc @will-moore @jburel

Testing the content of the DB via HQL, with this PR

(.venv3) bash-4.2$ omero hql 'select r from Roi r where r.id=276723'
Using session for user-7@localhost:4064. Idle timeout: 10 min. Current group: private-1
 # | Class | Id     | details             | name             | image         
---+-------+--------+---------------------+------------------+---------------
 0 | RoiI  | 276723 | owner=458;group=453 | primary__test 88 | ImageI:125215 
(1 row)

vs without

(.venv3) bash-4.2$ omero hql 'select r from Roi r where r.id=5400'
Using session for user-7@localhost:4064. Idle timeout: 10 min. Current group: private-1
 # | Class | Id   | details           | image       
---+-------+------+-------------------+-------------
 0 | RoiI  | 5400 | owner=58;group=53 | ImageI:4815 
(1 row)

To see details for object, enter line number.
To move ahead one page, enter 'p'
To re-display list, enter 'r'.
To quit, enter 'q' or just enter.

Good to merge from my perspective. I am also assuming this was an oversight but leaving others to comment.

@chris-allan
Copy link
Member

Do we need to do anything else on this PR or can it be merged, @sbesson?

/cc @joshmoore, @jburel

@jburel
Copy link
Member

jburel commented Jun 22, 2020

Looks good. iviewer RFE created ome/omero-iviewer#326

@sbesson sbesson merged commit a1dc193 into ome:master Jun 22, 2020
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.

4 participants