-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use 'Roi' for name of RoiColumn #56
Conversation
In terms of API, omero-metadata/src/omero_metadata/populate.py Line 2124 in 27cc728
ROI , Roi or roi should all be valid headers of a RoiColumn and the downstream code should not make any assumption on the capitalization of these column names.
I think the primary case where the population code needs to make a decision is when the input CSV specifies a ROI name and the matching ROI ID is populated as an extra column. In this case, this PR adjusts the code to use a convention similar to the one we adopted in #10 which is completely fine. No objection to having the example snippets of the README unified to use capitals. Should the other headings be adjusted accordingly? |
I think it will be important to know that ROI columns are named |
I wanted to check with @erindiel that using |
For complete reference, the PR that @will-moore is referring to in #56 (comment) is ome/openmicroscopy#5832. No objection to a PR that would aim the column names from my side but I think will need to make a few decisions:
Alternatively, would a case-insensitive bulk annotation column query be an option i.e. loosen the assumptions made by the client to meet the ones made in this plugin? |
Thanks @will-moore. It will be an easy switch to ignore case or assume uppercase, based on what you decide here. |
With those commits:
In some ways I like the idea that e.g. Also, if it applies in some API calls, e.g. For all the webclient changes discussed, I'll open a PR... |
I need to look more into ome/omero-web#277 but I think the combination of being strict on writing and lenient on reading as proposed is generally the safest approach to these problems as long as it does not result into performance degradation. As we are using both |
I don't think we're using |
I think the difference is
I think it's equally fine to use either form, I was just wondering whether we should handle |
Tested a few HCS and non HCS use cases with a combinations of headers/no-headers.
For a single plate with 4 wells:
For an image with 2 ROIs
In all cases, the column names populated by either versions of these CSV were identical with the only variation being the ordering of the columns. All the Overall, I think normalizing the name of our specialized columns makes complete sense. The only scenario I can think of where this might lead to confusion is if another
The only remaining place that might be worth updating is the In terms of test coverage, can a few asserts be added to the integration tests similarly to d511b69 to check columns with the correct names are created? Then I think this should be good to merge. |
@sbesson Apologies for dropping this for a while. I'm not quite sure what you mean by |
Hmmm - The build/tests don't seem to be running on that last commit.... ? |
Sorry I meant
Yes the GitHub workflow auto-disabled itself after 60 days of inactivity. I re-enabled it manually. Still considering how to automate this across the board as this is causing periodic issues |
This uses the name "Roi" for the
RoiColumn
, using the same convention we have adopted of "Image" for theImageColumn
.I noticed @sbesson had 'Roi' column on his screenshot at ome/omero-web#264 (comment)
I was looking at loading a row for a particular ROI using:
To test:
roi
).Populate_Metadata.py
script on this image (can use the webclient UI and this also tests Populate metadata Image dtype omero-scripts#184).Roi
(notroi
) - and that rest of the table is correct.