Closed Tom-TBT closed 1 month ago
Thanks @Tom-TBT
Checks are failing with flake8 omeroweb/webgateway/views.py:1657:23: F821 undefined name 'long'
Hi Will,
I am a bit offended by flake8 because literally four lines above there is another call to long
💀
And it seems rather an issue with flake8, as long
is defined with the imports:
https://github.com/ome/omero-web/blob/ab843f1acaf4404160ffbf916f1e352882ff19d9/omeroweb/webgateway/views.py#L64-L67
Maybe something with the try except
that flake8 isn't handling well with imports.
Anyway, I read about long
to figure out why it's in use here, and it just seems Python2-related.
Since Python 2 isn't supported anymore, should I get rid of long
completely?
Yes, just use int
instead.
NB: this import block has been removed in https://github.com/ome/omero-web/pull/531/files where we cleaned up a lot of the legacy Python2/3 compatibility code. If you merge origin/master
into your branch (which is what the CI build is doing under the hood), you will be able to reproduce the issue locally
The field index also seems to work as expected, without additional change.
I was wrong. There is still the issue when there's a different number of WellSample per Run between Wells. Note that the issue existed before these changes.
Discussing it locally, we figured that there are several use cases where this occurs (we have such datasets):
Here is the test data for the problem I described bellow Test_uneven_wells.zip
Four wells, and a first Run with 3 sample in C3 & D4, and 1 sample in C4 & D3.
A second Run with 1 sample for each
Selecting the second Run, there are three fields listed (when there should be only one). First field shows C4 & D3, second shows nothing, third shows C3 & D4:
This is because the Field# is matched to the WellSample index (indexing done per Well):
Well | ws index | Acquisition
-----------------------------
C3 | 1 | 1
C3 | 2 | 1
C3 | 3 | 1
C3 | 4 | 2
C4 | 1 | 1
C4 | 2 | 2
D3 | 1 | 1
D3 | 2 | 2
D4 | 1 | 1
D4 | 2 | 1
D4 | 3 | 1
D4 | 4 | 2
Selecting Acquisition 2, the index is in the range[2,4], given by plate.getNumberOfFields(run_id)
, and mapped to a field index this way:
https://github.com/ome/omero-web/blob/4d8d25ef7528e49cadfb5b777a47f6953c198fba/omeroweb/webclient/forms.py#L384-L388
So in the case of the second run:
Field#1 -> index 2 (C4 & D3)
Field#2 -> index 3 (no such index at Acquisition 2)
Field#3 -> index 4 (C3 & D4)
where indexes are used by this query: https://github.com/ome/omero-web/blob/4d8d25ef7528e49cadfb5b777a47f6953c198fba/omeroweb/webgateway/plategrid.py#L49-L61
I am investigating how I can solve this issue. Now trying with HQL queries to get the index adjusted to the Run:
SELECT
w.id,
index(ws) - (
SELECT MIN(index(ws_inner))
FROM Well w_inner
JOIN w_inner.wellSamples ws_inner
WHERE ws_inner.plateAcquisition.id = :runid
AND w_inner.id = w.id
) AS adjusted_index
FROM Well w
JOIN w.wellSamples ws
WHERE ws.plateAcquisition.id = :runid
So, @Tom-TBT you want the number of Fields for any Acquisition to be the max(field_count)
for that acquisition.
In your example, the field_count
for all Wells for Acquisition2 is 1, so there should only be a single Field in the UI?
As was mentioned yesterday, some tests are failing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/382/#showFailuresLink
They are mostly failing with:
TypeError: __init__() missing 1 required positional argument: 'acqid'
with also one of:
> if self.acquisition > 0:
E TypeError: '>' not supported between instances of 'str' and 'int'
Tom, in the screenshot above where you've annotated the screenshot in Red and Green Run1
and Run2
, I think it would be really useful if the UI could do this for you. When you're loading well children
without filtering by Acquisition E.g. /webgateway/well/19106/children/
you'd need to add the Acquisition info to each Image returned, e.g.
"acquisition": {"id": 123, "name": "01 - Uneven Run"}
Obviously the UI changes would need a bit of thought about the best way to show the different Acquisitions that the Fields belong to, so it doesn't need to happen in this PR (although could maybe add it as a tooltip in this PR to help debugging)?
@will-moore
you want the number of Fields for any Acquisition to be the max(field_count) for that acquisition.
Yes exactly. I implemented that in my last commit in webclient/views.py
I now pass to WellIndexForm
the field range in [0, max(field_count)], and I modified the query in PlateGrid
to return the WellSamples at these "offset indexes".
the field_count for all Wells for Acquisition2 is 1, so there should only be a single Field in the UI
Yes, that's what we should see now.
And with the fix on PlateGrid.__init__
, the tests shouldn't fail.
@sbesson
I changed PlateGrid init for backward compatibility. I expect the behavior to be identical to what it was before my changes.
I added doc about acqid
to explain that fid
should be 0-indexed for the given acquisition (when using acquisition).
Some notes (for myself at least) about the UI changes you proposed Will:
Can add Run information in here. https://github.com/ome/omero-web/blob/99aebd2a6bc892db0ac38f791821f17886a59095/omeroweb/webgateway/views.py#L1833-L1839
Something like m["name"] += f" Run:{run_name}"
would append to the tooltip the name of the run, for the HTML generated here:
This looks good now, using the sample file above:
Code looks good to me
Follow up on https://forum.image.sc/t/omero-web-plate-display-with-multiple-plateacquisition/90071
Rather than switching the plate display using the JSON api (my understanding is that it would require rewritting quite of JS to populate the plate with the results), I figured that I was able to pass the plateAcquisition ID (already exists and in use to filter the content of the "bird eye" in
plate.html
).Passing the plateAcquisition ID down to
PlateGrid
, it adds a filter on which WellSamples are loaded.plate.html
does the rest by populating only the Wells that contains WellSamples. Further selection of a well will show in thebird eye
only the WellSample of that acquisition (as it already did).If no plateAcquisition is passed (that's the case when the plate is selected in the tree), then all wellsample are loaded (as it did before). The difference in this case is that there is no filter on the "bird eye" when selecting a well, so all wellsamples are shown (which is the expected behaviour).
For it to show the plate when there is more than one run, I just removed the logic that prevented the plate to be loaded with more than one run (which was not preventing extra loading, since everything was loaded when selecting a plateacquisition)
The
field index
also seems to work as expected, without additional change.