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

Query timestamps from all planes when C or Z dimensions are not filled #486

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

Tom-TBT
Copy link
Contributor

@Tom-TBT Tom-TBT commented Oct 11, 2024

Hello,
we found a bug on the timestamps display. We have data where the channel at index 0 was acquired only for the first and last frames.

Because the query to get timestamps was restricted to Z=0 and C=0, most timestamps were missing: we only got a list of two (instead of ~400).
-> Consequently, the last timepoint was assigned to the frame n°2, while all other frames had NaN values for timestamps.

To fix it, I added a condition that if the number of timestamps returned from the query is smaller than the number of frames, we do a second query. As such, it will impact performance (and results) only for images requiring it.

Return one arbitrary plane per theT. It does not ensure that it's the lowest theC/theZ, but seems acceptable for those edge cases.

FROM PlaneInfo Info WHERE Info.pixels.id = :pid 
AND Info.id IN (
    SELECT min(subInfo.id) FROM PlaneInfo subInfo
    WHERE subInfo.pixels.id = :pid GROUP BY subInfo.theT
)

(ensuring here that the minimum possible theC/theZ combination is returned increases the complexity of the query by a lot, because min(theC) is not always the same plane as min(theZ))

params = omero.sys.ParametersI()
params.addLong('pid', image.getPixelsId())
query = """
from PlaneInfo Info where Info.pixels.id=:pid
Copy link
Member

Choose a reason for hiding this comment

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

it's harmless, but you don't need this where Info.pixels.id=:pid as you're selecting PlaneInfo by the same criteria below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I remove it for simplicity.

@will-moore
Copy link
Member

I don't suppose you have a sample image that you can share to help with testing? Otherwise I can just test the query locally

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Using a sample image, I can reproduce the bug and tested that the fix works as expected.

@will-moore will-moore merged commit b4d70c9 into ome:master Nov 19, 2024
1 check passed
@will-moore will-moore added this to the 0.15.0 milestone Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants