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

Zarr Sink Frame Values #1625

Merged
merged 31 commits into from
Oct 22, 2024
Merged

Zarr Sink Frame Values #1625

merged 31 commits into from
Oct 22, 2024

Conversation

annehaley
Copy link
Collaborator

@annehaley annehaley commented Sep 4, 2024

Resolves #1325. This PR adds frame values to the Zarr sink, which allows a user to specify true values for each index of a frame axis. This can be done with addTile or by setting frameValues directly. Additionally, the user may set a dictionary of frameUnits to describe the units used by each frame axis and/or a list of frameAxes to assert an ordering to the frame axes.

For example:

import large_image_source_zarr
import numpy as np

sink = large_image_source_zarr.new()
sink.frameAxes = ['z', 't', 'm']
sink.frameUnits = dict(z='cm', t='ms', m='mm')
for z, z_value in enumerate([2, 4, 6, 8]):
    for t, t_value in enumerate([10.0, 20.0, 30.0]):
        t_value += 0.01 * z
        for m, m_value in enumerate(['r', 'g', 'b']):
            random_tile = np.random.random((300, 400, 3))
            sink.addTile(
                random_tile,
                0, 0,
                z=z, t=t, m=m,
                z_value=z_value, t_value=t_value, m_value=m_value
            )
print(sink.getMetadata())

This PR also uses the frame values in the slider labels shown in the Frame Selector component. The following video demonstrates the value labels on axis sliders for a randomly-generated image using the new Zarr sink API.
C and Z have uniform values, while T has non-uniform values. (This image was generated with the code in testFrameValues in test/test_sink.py.)
https://github.com/user-attachments/assets/4b41bcd3-8164-46f8-b78b-9d919cecefa0

@annehaley annehaley force-pushed the zarr-sink-axis-values branch from 299dbe5 to f8198a8 Compare September 4, 2024 19:31
@annehaley annehaley force-pushed the zarr-sink-axis-values branch from f8198a8 to 6c00eed Compare September 4, 2024 20:01
@annehaley annehaley marked this pull request as ready for review September 10, 2024 17:31
@annehaley annehaley requested a review from manthey September 10, 2024 17:31
@manthey
Copy link
Member

manthey commented Oct 8, 2024

If you have a multi-band tile and specify a c_value, it doesn't work as expected. That is

ts = large_image.new()
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=0, z=0, z_value=1, c_value='DAPI')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=1, z=0, z_value=1, c_value='CD4')

fails, but without the c_value succeeds.

And, if this is a 1 band tile, then when I try to print the metadata, it fails:

ts = large_image.new()
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, c=0, z=0, z_value=1, c_value='DAPI')
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, c=1, z=0, z_value=1, c_value='CD4')
pprint.pprint(ts.metadata)

@manthey
Copy link
Member

manthey commented Oct 8, 2024

Another use case I was expecting to work:

ts = large_image.new()
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, z=0, z_value=1)
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, z=1, z_value=3.2)
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, z=2, z_value=6.4)
ts.addTile(np.zeros((100, 100, 1)), x=0, y=0, z=2, z_value=6.3, c=1)

@annehaley
Copy link
Collaborator Author

The first two snippets you provided have been added as a pytest and fixed in c85ebf0.
The third snippet you provide is a use case that is already not supported by the addTile function. The current implementation of addTile requires that all axes are declared upon the first call, and a user cannot add new axes later. While outside the scope of this PR, this use case is reasonable and should be addressed in another PR. I have created #1674 to track this.

@manthey
Copy link
Member

manthey commented Oct 9, 2024

I had another example that had been failing during addTile, but with the latest changes now fails when getting the metadata:

ts = large_image.new()
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=0, z=0, z_value=1, c_value='DAPI')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=0, z=1, z_value=3.2, c_value='DAPI')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=0, z=2, z_value=6.3, c_value='DAPI')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=1, z=2, z_value=6.4, c_value='CD4')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=1, z=1, z_value=3.1, c_value='CD4')
ts.addTile(np.zeros((100, 100, 3)), x=0, y=0, c=0, z=1, z_value=1.1)
pprint.pprint(ts.metadata)

My guess is that this is due to unpopulated values in some combinations.

@manthey
Copy link
Member

manthey commented Oct 9, 2024

Currently, the metadata entries for Value<axis> is in the zarr source code. I think it would be better to move it to the _addMetadataFrameInformation method in the base class so if these values are populated in frames in non-zarr sources, the aggregated values will still be added to the metadata.

@manthey
Copy link
Member

manthey commented Oct 9, 2024

Should we cast the values stored in each frame to native python float or int datatypes rather than leave them as numpy data types? As long as the native numpy data types serialize to json, it probably doesn't matter, but for consistency between sources, we may want to do the cast.

@annehaley annehaley force-pushed the zarr-sink-axis-values branch from 9a8e7d1 to 1009eb1 Compare October 10, 2024 13:02
@annehaley annehaley force-pushed the zarr-sink-axis-values branch from 63c5eea to 385a9f4 Compare October 10, 2024 21:20
@annehaley annehaley force-pushed the zarr-sink-axis-values branch from 385a9f4 to db43213 Compare October 10, 2024 21:28
@annehaley annehaley force-pushed the zarr-sink-axis-values branch from 845e8ad to b3fde84 Compare October 16, 2024 17:20
@manthey
Copy link
Member

manthey commented Oct 21, 2024

I'd like to have issues for two follow ups to this PR:

  • Setting values in multiple processes doesn't appear in the final result. That is, I think only the last process to white the zarr has sets values.

  • The display precision should probably be limited to some number of significant digits. For instance, in a test example I get
    image

@annehaley
Copy link
Collaborator Author

@manthey Since the value precision for the labels is a quick fix, I added that change in e41e125. We can address the multiprocessing case in a follow-up PR as you suggested. I made this issue to track it: #1698.

@manthey
Copy link
Member

manthey commented Oct 22, 2024

@manthey Since the value precision for the labels is a quick fix, I added that change in e41e125. We can address the multiprocessing case in a follow-up PR as you suggested. I made this issue to track it: #1698.

Great. When CI passes, merge away.

@annehaley annehaley merged commit a4160ea into master Oct 22, 2024
18 checks passed
@annehaley annehaley deleted the zarr-sink-axis-values branch October 22, 2024 16:47
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
manthey added a commit that referenced this pull request Oct 25, 2024
Ensure reading non-editable zarr files the c axis, if present, has a
stride of 1.  PR #1625 changed this.
@manthey manthey mentioned this pull request Oct 25, 2024
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.

Axis conceptual values
2 participants