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

Support images_by_name for Screen and Plate #64

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Dec 16, 2021

See #63.

Supports parsing Image Name to Image ID for HCS data. Image Name must be unique to a Plate (similar to Dataset).

NB: There has been a long-standing inconsistency in the way that populate metadata handles column names for PDI vv HCS.
For PDI, you start with "Image Name" column (with s type in header) in your csv. A new "Image" column is added with Image IDs.
This seems to make the most sense.
For HCS, you start with "Well" column (with well type in header), that contains the Well Names. This column is converted to a Well ID column (still named Well) and a new Well Name column is added, containing the names that were in the Well column. This is confusing!

In this case, so as not to have different handling of Image Name columns for HCS vv PDI, we need an Image Name column (type: s) alongside a Well column (type: well), even though they both contain names!

E.g. screen.csv

# header plate,well,s,s
Plate,Well,Image Name,Drug_name
plate1,A1,well 1 field 1, DMSO
plate1,A1,well 1 field 2, DMSO
plate1,A2,well 2 field 1, Drug1
plate1,A2,well 2 field 2, Drug1

E.g. plate.csv

# header well,s,s
Well,Image Name,Drug_name
A1,well 1 field 1, DMSO
A1,well 1 field 2, DMSO
A2,well 2 field 1, Drug1
A2,well 2 field 2, Drug1

TODO: add tests...

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#988. See the console output for more details.
Possible conflicts:

--conflicts

@will-moore
Copy link
Member Author

will-moore commented Jan 7, 2022

I think I need to get #62 merged ahead of this PR... Will look at that now...

@snoopycrimecop
Copy link
Member

snoopycrimecop commented Jan 8, 2022

Conflicting PR. Removed from build OMERO-plugins-push#1005. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build OMERO-plugins-push#1383. See the console output for more details.

@will-moore
Copy link
Member Author

Closing for now to avoid more conflicts...

@will-moore will-moore closed this Jun 13, 2022
@JensWendt
Copy link

Will this be re-opened at some point?
Still hoping for the possibility to add ROI specific measurements to multiple images (aka positions) in one well.

@will-moore will-moore reopened this Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@ome ome deleted a comment from snoopycrimecop Jan 4, 2023
@will-moore
Copy link
Member Author

Re-opened and fixed merge conflicts. Hopefully tests will pass (not run locally yet).

@JensWendt it would be a great help if you can test this (see description) and let us know if it's working for you?
Thanks!

@JensWendt
Copy link

Heyo @will-moore,
thank you!!
Will test it 👍
Which description? The one in the first post?

@will-moore
Copy link
Member Author

Yes, with the screen.csv and plate.csv examples. Thx

@JensWendt
Copy link

Heyo @will-moore ,

I did some testing.
With the CLI omero metadata populate ... and as well with the server side script Populate Metadata.
I simply exchanged the current populate.py in /opt/omero/venv_server/lib/python3.6/site-packages/omero_metadata with your changed one from this branch. I think this was the correct way to test it!?
Everything works as expected with one exception.
screen.csv will produce the expected omero-table but will throw the warning WARNING:omero_metadata.populate:PlateColumn is unimplemented.
The result looks like this, without the Plate column hyperlink functionality.
grafik

From my perspective this is not a big problem as long as the table is correct.
Good job!
Thank you!!

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.

A few questions on the implementation but no concern from my side.

Soince the tests have been modified, have we also have validated that the current workflow i.e. being able to annotate plate/screen without any image column is preserved? And is it still tested?

@@ -742,18 +750,22 @@ def _load(self):
self.images_by_id = dict()
images_by_id = dict()

self.images_by_name = dict()
images_by_name = dict()
Copy link
Member

Choose a reason for hiding this comment

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

What's the goal of this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

That creates a dictionary that is assigned to self.images_by_name[self.target_object.id.val] and is then passed to the method below where it's populated. This is just following the same pattern as for images_by_id = dict() in the lines above.

src/omero_metadata/populate.py Show resolved Hide resolved
@JensWendt
Copy link

I was playing around with ROI specific metadata for whole projects/screens.
It works but - again - gives me the warning WARNING:omero_metadata.populate:PlateColumn is unimplemented. for a .csv with a Plate column containing plate names.
The bulk annotations looks like this:
grafik

No hyperlink functionality for either Plate or ROI. Again, no problem for me, but I thought it might be interesting to report.

If the pull request goes through maybe also adapt the readme.rst line
"If the target is an Image or a Dataset, a CSV with ROI-level..."
Because now it seems to work on every level 👍

@abhamacher
Copy link

Hi everyone,

I also did some testing this morning with the following combinations of source csv data containing either:

  • plate, well
  • plate, well, image
  • plate, well, image, roi (only with the current github version of populate.py)

Except of the warning message, that Jens already mentioned, I would like to point out the following concerns:

  1. When populating image-based data an "Image Number" is added to the OMERO.table. But this number is not the Image ID, which I find a bit confusing.

  2. When populating image-based data, OMERO.parade can properly load individual image data for example in the filter menu (see Example 68...366 per image) but the dots in the parade plot do not cover all images, but rather wells only, which is inconsistent.

(as a side note, with the same OMERO.table plots in parade-crossfilter show all individual image dots)

  1. Based on concern no. 3, using parade one cannot see if the data shown is coming from wells or from images. Typically someone would populate the OMERO.table but another or multiple persons would browse through the data, which are not aware of the underlying data structure. So this is something that should be visible somehow to the user.

  2. When populating roi-based data, the activity log returns an irritating message ("Failed to get results"), although the OMERO.table seems to be generated properly.
    populate1

  3. Issue no. 2-no. 4 also apply to roi-based data.

  4. I'm not a big fan of the order of columns. For example, placing well id at the beginning of the OMERO.table but well name and plate name at the very end? I typically use Cellprofiler output for uploading metadata, which has a lot of columns and therefore results into lots of scrolling to the right, if I want a readable well name, for example.

  5. Another side note for @will-moore and parade-crossfilter: when selecting one row of the table view panel of roi-based data, only one specific image is shown in the image preview panel (which is therefore fine). However, image-based data results in the preview of all fields of the wells, instead of a single image (which should be basically selected in the table view by selecting a single row).

I hope my notes are not too confusing and a bit helpful.

BR, Anna

The script is working fine, so it looks like these are invalid warnings
but it is hard to understand the working of the script
@will-moore
Copy link
Member Author

@abhamacher "Image Number" - do you mean the "Image" column? That is definitely supposed to be the Image ID. If it wasn't then the script is failing badly. Do you have some sample data (csv)?

I think omero-parade doesn't handle multiple Images per Well and certainly not multiple ROIs per Image.

omero-metadata always adds Columns at the end rather than inserting them.

I don't quite understand what 8) is about. Could you create a new issue for this on that repo (and same for any others above if you want, on their respective repos).

In the last commit above, I have reduced the logging level for warnings.

@abhamacher
Copy link

Hi Will,

I apologize for mixing up topics and bringing some confusion to this thread. As you can see, I added parts of the mentioned issues at the parade and parade-crossfilter repo. About the "ImageNumber" -- that was a mean one, it actually came from the original Cellprofiler data. Sorry, about that, so it has nothing to do with omero-metadata.

omero-metadata always adds Columns at the end rather than inserting them.

Well, still this is not very handy/ well readable for users. Especially because I add for example the plate and well names to the upload csv-file as the very first columns, but omero-metadata exchanges them to IDs and adds the names to the end of my table. Why can't the names just remain where they are (at the beginning) and the IDs being added to the end of the table?

Thanks, Anna

@will-moore
Copy link
Member Author

Hi Anna,
yes I agree that's annoying behaviour for HCS data (as I described in this PR description) - The behaviour for Project/Dataset/Image is better - in that case the "Image Name" column is unchanged and an "Image" ID column is simply added.
I have previously created an issue for this at #29
Feel free to add comment there.

@JensWendt
Copy link

Hi @abhamacher ,

regarding point 5) --> I've been running into this issue also with other scripts.
See my recent thread https://forum.image.sc/t/failed-to-get-results-error-while-running-scripts/76459 and the two other threads referenced in the first post there.
It seems to be an issue with processes which take a lot of time or involve a lot of "entities", that at some point something goes wrong and you may or may not get the desired end result after seeing the "Failed to get results" statement.

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#372. See the console output for more details.
Possible conflicts:

--conflicts

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.

5 participants