Bulk annotations right panel

[will-moore]

This is metadata "Bulk Annotations" PR #3875 rebased onto develop (actually it was on develop originally as #3863 but not merged).

Please see #3875 for main testing info.

NB: Also tweaked to add this under "Tables" collapsible tab. To test:

• When "Tables" tab is not expanded, the data is not loaded.
• When you expand the "Tables" tab, data will be loaded
• If the "Tables" tab is expanded, selecting new Well will load new data.
• "Info" should give tooltip (currently not working - known issue)

[will-moore]

Do we need to update our Docs or Help guides with this functionality, since this is otherwise a "hidden feature"? cc @hflynn @gusferguson. Only described in our testing scenario at https://github.com/openmicroscopy/ome-internal/blob/master/testing_scenarios/BulkAnnotations.txt

[joshmoore]

The ImageJ folks were certainly asking recently about "how do I upload a table from ImageJ to have it shown in the right-hand panel".

[gusferguson]

Will this ever be "User" functionality or will it really be an Admin/Facility Manager/Developer functionality?

[joshmoore]

Probably not a completely vanilla user concept until some file formats come with pre-defined bulk annotations that the users will want to see in the UI. For the moment, perhaps more power-users who are willing to add a file in the proper format to add this value.

[jburel]

@gusferguson: it will probably be like roi-folder first use cases.
Table generated via a script and viewable in the clients

[emilroz]

@will-moore without extra join fetch links.details.owner this query will not load owner details when working in none default groups, which results in exception later on in the code.

[will-moore]

Thanks @emilroz, but it seems to be loading link owners already.
In the json at webgateway/table/Plate.wells/2061/query/?query=Well-2061 I see
"addedBy": "Will Moore"
even when logged in as another user whose default group is different from this one.

[emilroz]

OK, that wasn't my experience at all. I could only load this is the user's default group was set to the group where the data was. @will-moore are you testing with 5.2?

[will-moore]

Yes, on develop branch.

[emilroz]

All the test I've done where on 5.1 so maybe there's a difference to how the objects are loaded now.

[will-moore]

@emilroz I went to add a test for loading of table data for plate. Although I didn't test the permissions in the test itself, I found that for root user to query data from another user in a private group does need to fetch links.details.owner. Thanks!

[will-moore]

OK, so we probably need some code examples in Python, Java and Matlab.

[will-moore]

@pwalczysko That commit will fix the duplication in right panel (if not the underlying cause).

[will-moore]

Also need to check that Image Viewer shows the same data (under "Image Information")

[hflynn]

@gusferguson what is your thinking here? Have you made a Trello card for adding this functionality to a help guide?

[pwalczysko]

@will-moore : Yes, the #4446 (comment) duplication problem is gone. The image Viewer info #4446 (comment) check I have done previously and confirmed today that the info is displayed and okey.

[will-moore]

Discussed with @pwalczysko: The webclient functionality & tests are working OK.
We have other PRs for Python code samples and an OMERO.script for populating metadata. We just need to have some 'help' docs on how to use this cc @gusferguson.

The only outstanding question from above is @jburel asking about performance, of "loading the full table". I don't know what our criteria should be for performance: we maybe want to test with a moderately large file, but I don't think a table with 370K rows is realistic since this scenario is one row per well, E.g. 364 max. Although we do openTable(file) to open the file, we only query a row at a time for display in webclient.

@joshmoore Should I generate a large csv file (how large) and test loading speed? If so, how slow is too slow? Or, are we happy that the strategy here is the optimum way of reading tables? NB: this PR doesn't change any of the tables queries since they were already in use for the image viewer (and have been in use on the IDR for some time).

[emilroz]

@will-moore one thing to keep in mind here, is that the table can be attached also to the screen. So for example: http://jcb-dataviewer.rupress.org/jcb/browse/7555/ you could have a table with: 141 plates, 3 fields, 96 wells: 41K rows. We have examples where number of fields is more then 10 with 384 wells so we can easily talk here about 100's of thousands of rows per table.

[will-moore]

@emilroz This PR doesn't support use-cases with multiple Fields (rows) per Well, and I'm not sure if (and populate_metadata.py does? But I can see that it's possible to support multiple Plates per Screen.

[will-moore]

NB: I noticed that Tables on Screens breaks when the Plate is in multiple Screens.

    serverStackTrace = ome.conditions.ApiUsageException: Query named:
    select obj0 from Screen obj0
...
has returned more than one Object
findBy methods must return a single value.

For now, I will fix this to use findBy query, and pick the first Screen.
But this comes back to the question discussed with @pwalczysko above about improving the display of multiple tables for each well. But that should be handled in a future PR.

[will-moore]

To test the last commit, follow the Bulk Annotation scenario for a table on a Screen, then put the Plate into a second Screen and check that the tables data is still displayed OK on Wells.

[emilroz]

@will-moore we have added support for Images in this commit: joshmoore@29b9ddd
so it is possible to resolve the metadata on Images instead of wells. I have the changes ready so I can add the commits that introduce the Image queries as well.

[pwalczysko]

The test of the last commit passed #4446 (comment).

I suppose some further discussion should happen between @will-moore and @emilroz considering the steps here.

[will-moore]

Thanks @emilroz. I am away Monday & Tuesday, and it would be nice to get this PR in, so I can begin on rebasing #3896. Could we add those commits into a different PR?

[will-moore]

@joshmoore Is there anything else needed for this PR? See questions from #4446 (comment)
Otherwise, it would be great to get this merged so that I can start rebasing #3896 on top of it.

[joshmoore]

@joshmoore Should I generate a large csv file (how large) and test loading speed? If so, how slow is too slow?

Sorry, I missed the direct question on me, @will-moore. From my side, this is currently working in "production" but I leave suggestions for pounding it further on the mainline to others if they have them.

I think probably more necessary is a final code review. @chris-allan had pointed out the change in marshal.py as a possible concern. Perhaps you could go through and explain any changes that might need to be changed in the doc, @will-moore? Just need to make sure the impact for a point release is within bounds.

[will-moore]

I am happy to revert the change to marshal.py. Agreed, it is an API change, although it could be considered a bug fix! I just found that because of a script I had an image in a Well and a Dataset (E.g. you can get this with Dataset_To_Plate script) which broke the display of bulk annotations in the viewer since imgData json only handles a single Parent.

[will-moore]

@joshmoore @chris-allan Reverted the changes to marshal.py.

[chris-allan]

For what it's worth we have the exact same problems at the moment with the PathViewer. If images are in multiple datasets, as the image data JSON only currently supports a single parent, things quickly go off the rails.

What I'd suggest is a semi-additive change where the first Dataset and WellSample are included in the existing fields, rather than the last as in 0c02e69, and we include a list of all parents under the parents or similar key. This is "breaking" but only as far as semantics are concerned and is in line with the intentions of the existing JSON. What we should examine quite closely though is the use of these keys in the full image viewer before making any change at all.

/cc @knabar

[will-moore]

@chris-allan I decided to handle parents in imageMarshal in a different PR, so as not to hold up this one #4486

[jburel]

"API" PR not included in this release, use cases/implementations to be discussed.
Merging this PR