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.

Sign up for GitHub

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

Jump to bottom

Ome zarr v0.3 tweaks #104

Merged
merged 11 commits into from
Aug 12, 2021
Merged

Ome zarr v0.3 tweaks #104

merged 11 commits into from
Aug 12, 2021

Conversation

will-moore
Copy link
Collaborator

@will-moore will-moore commented Jul 12, 2021 •
edited
Loading

See #103

Use the axes data for dimension names and don't assume data is 5D.

This PR build is deployed at https://deploy-preview-104--vizarr.netlify.app/
These currently all working except 2D example:

@will-moore will-moore mentioned this pull request Jul 12, 2021
Closed
@joshmoore
Copy link

cc: @constantinpape @tischi

@will-moore will-moore mentioned this pull request Jul 13, 2021
Merged
@manzt
Copy link
Member

manzt commented Jul 13, 2021

@will-moore - merged #105

@will-moore will-moore mentioned this pull request Jul 14, 2021
Merged
@manzt
Copy link
Member

manzt commented Jul 16, 2021

Thanks for this PR! @will-moore - is this ready for review?

@will-moore
Copy link
Collaborator Author

Yes, I think it's ready for a look. I was hoping to prepare some v0.3 test data and get it public but I ran out of time and now I'm on holidays for 2-3 weeks, so you might want to wait a bit.

@manzt
Copy link
Member

manzt commented Jul 17, 2021

Great! Enjoy the well deserved holiday 🙂

manzt
manzt reviewed Jul 26, 2021
View reviewed changes
Copy link
Member

@manzt manzt left a comment

Choose a reason for hiding this comment

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

Nice! Just a few comments about organization. In general, I'd prefer that all parsing of axes for OME-data lives in ome.ts. Curious to hear your thoughts.

src/ome.ts Outdated Show resolved Hide resolved
src/ome.ts Outdated Show resolved Hide resolved
src/io.ts Outdated
@@ -121,6 +110,9 @@ export async function createSourceData(config: ImageLayerConfig): Promise<Source

if (node instanceof ZarrGroup) {
const attrs = (await node.attrs.asObject()) as Ome.Attrs;
if ('multiscales' in attrs && attrs.multiscales?.[0]?.axes) {
config.axis_labels = attrs.multiscales?.[0]?.axes;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what I think about overriding config.axis_labels here. I think we can just inspect the multiscales metadata in loadOmeroMultiscales and derive the axis labels for well and plate from the representative image with multiscales.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've moved this to ome.ts in 8ed9c89 but this doesn't handle the case when we have axes data but no omero data (so loadOmeroMultiscales() is not called). E.g. sample data at https://oc.embl.de/index.php/s/KLTaSgX1c4zgnHP
Don't know if data with axes should be handled by ome.ts too?

src/ome.ts Outdated Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved
@will-moore will-moore mentioned this pull request Aug 5, 2021
Merged
@manzt
Copy link
Member

manzt commented Aug 5, 2021 •
edited
Loading

@will-moore can you rebase on main?

git switch main
git pull # pull down latest changes
git switch ome_zarr_v0.3_tweaks
git rebase main # should just auto rebase
npm install # install latest dependencies
git push --force # need to force push to github

@joshmoore joshmoore mentioned this pull request Aug 6, 2021
Merged
@will-moore
Copy link
Collaborator Author

I'm seeing an intermittent failure e.g. https://deploy-preview-104--vizarr.netlify.app/?source=https://minio-dev.openmicroscopy.org/idr/v0.3/idr0062-blin-nuclearsegmentation/6001240.zarr/

Sometimes this works:

Screenshot 2021-08-06 at 16 26 44

But even with a refresh, it can fail:

Screenshot 2021-08-06 at 16 27 51

@manzt
Copy link
Member

manzt commented Aug 6, 2021

Having a look now. I can't seem to reproduce locally with npm run build && npm run preview... I added sourcemaps and unminified the build to hopefully debug better. Curious if its something with netlify preview.

@manzt manzt mentioned this pull request Aug 6, 2021
Closed
@will-moore
Copy link
Collaborator Author

Thanks @manzt. I'm only seeing that error with the netlify preview, so maybe we can ignore or workaround for now. Just didn't want all the broken links above to be a blocker.

The main question that I have now is where to handle the axes info for OME-NGFF that don't have omero data (so aren't handled by loadOmeroMultiscales(). Maybe we do loadOmeroMultiscales() if we have omero OR axes data (but neither are essential)? And this could be renamed to loadOmeMultiscales().
This would mean that utils.loadMultiscales() is never used except for v0.1 and v0.2 data that are also missing omero. And loadOmeMultiscales() could also handle this if it can handle the missing omero and axes data.

I guess I'm saying that we don't need utils.loadMultiscales(), so if that seems OK I'll go ahead and see what that looks like...?

@manzt
Copy link
Member

manzt commented Aug 10, 2021

Ah, sorry @will-moore. I got lost in trying to debug the apparent netlify issue and missed your question in the comments.

I think what you have proposed sounds reasonable. I took a stab a cleaning things up a bit. Let me know what you think about the latest commit. Happy to revert if you have something else in mind.

manzt
manzt reviewed Aug 10, 2021
View reviewed changes
src/io.ts
axis_labels = getAxisLabelsFromMultiscales(attrs);
if (!config.axis_labels) {
// Update config axis_labels if present in multiscales
config.axis_labels = attrs.multiscales[0].axes;
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd rather just be explicit here for this edge case, rather than adding a util that might return undefined.

manzt
manzt reviewed Aug 10, 2021
View reviewed changes
src/io.ts
@@ -24,18 +25,6 @@ import {
RGB,
} from './utils';

function getAxisLabels(arr: ZarrArray, axis_labels?: string[], channel_axis?: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think channel_axis got lost when moving this to src/utils.ts. This is important for images without any dimension labels, as you can tell vizarr to separate out the channels in that dim like in napari.

@will-moore
Copy link
Collaborator Author

That looks great, thanks. Tested with the v0.3 links above and various v0.1 examples from the blog, as well as v0.3 data (without omero) https://oc.embl.de/index.php/s/KLTaSgX1c4zgnHP.

I also tried removing the omero data from multi-channel images, where one of the axes is labelled c.
Currently this is not considered a multi-channel image, and you get a c slider:

Screenshot 2021-08-11 at 09 15 03

So I'll look at fixing that...

@will-moore
Copy link
Collaborator Author

That commit uses labels.indexOf('c') to pick channel_axis for non omero images, similar to the approach for napari: ome/napari-ome-zarr#9

Screenshot 2021-08-11 at 10 20 23

@manzt
Copy link
Member

manzt commented Aug 12, 2021

Wonderful. I just tried it out! Going to go ahead and merge, thanks so much for working on this @will-moore.

cc: @joshmoore

@manzt manzt merged commit 0e02933 into hms-dbmi:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manzt manzt manzt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@will-moore @joshmoore @manzt