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

feature/time_interval #20

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feature/time_interval #20

wants to merge 2 commits into from

Conversation

BrianWhitneyAI
Copy link
Contributor

Description

The purpose of this PR is to resolve bioio-devs/bioio#35 and add a way to access time interval. The methodology I took was creating a new type TimeInterval and accessing similarly to PhysicalPixelSize. There is some consideration to add TimeInterval into the PhysicalPixelSize type.

@evamaxfield
Copy link
Contributor

I would argue that we should expand the physical pixel sizes object as such:

class PhysicalPixelSizes:
   # same as before

class DimensionSizes:
    t: float
    physical_pixel_sizes: PhysicalPixelSizes

that we don't break the existing API but we keep everything together in a single object.

I also am curious: are we sure that time intervals are constant? Can there not be time intervals that are say exponential? [t=0, t=1, t=2, t=4, t=8] etc.?

@BrianWhitneyAI
Copy link
Contributor Author

I would argue that we should expand the physical pixel sizes object as such:

class PhysicalPixelSizes:
   # same as before

class DimensionSizes:
    t: float
    physical_pixel_sizes: PhysicalPixelSizes

that we don't break the existing API but we keep everything together in a single object.

I also am curious: are we sure that time intervals are constant? Can there not be time intervals that are say exponential? [t=0, t=1, t=2, t=4, t=8] etc.?

I can confirm that time intervals will not always be constant as I have stitched together images with different time intervals. I think in general though to have intervals like this you would need a unit and right now these are just ambiguous "time units" which are not necessarily the same length of time.

@toloudis
Copy link
Contributor

This is a weird API thing, hard to know what's best here.

Eva's DimensionSizes is at least trying to encapsulate all the dimension measures together. But I'm not sure what value it really adds over the PR. (aside: I still wish our pixel sizes defaulted to 1.0 instead of None.)

I think the ome ngff 0.4 metadata has pretty decent handling of dimensions:

"axes": [
                {"name": "t", "type": "time", "unit": "millisecond"},
                {"name": "c", "type": "channel"},
                {"name": "z", "type": "space", "unit": "micrometer"},
                {"name": "y", "type": "space", "unit": "micrometer"},
                {"name": "x", "type": "space", "unit": "micrometer"}
            ],

and then the scale transforms are just in axis order:

"scale": [1.0, 1.0, 0.5, 0.5, 0.5]

where the scale is in per-axis units, so scale*pixels = physical size

Copy link
Contributor

@evamaxfield evamaxfield left a comment

Choose a reason for hiding this comment

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

Works for me!

@tyler-foster tyler-foster removed their request for review October 1, 2024 17:33
@SeanLeRoy
Copy link
Contributor

Decided to go with Dan's suggestion & keeping physical pixel sizes. Changes made in this PR which add a shorthand access to time also seem fine to keep.

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.

read physical time metadata
4 participants