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

Support uploading neuroglancer precomputed and N5 #7578

Merged
merged 15 commits into from
Feb 5, 2024

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Jan 22, 2024

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

  • Precomputed Multilayer
  • N5

Issues:


(Please delete unneeded items, merge only when none are left open)

@frcroth frcroth changed the title Support uploading single neuroglancer precomputed data layer Support uploading neuroglancer precomputed layers Jan 24, 2024
@frcroth frcroth changed the title Support uploading neuroglancer precomputed layers Support uploading neuroglancer precomputed and N5 Jan 29, 2024
@frcroth frcroth marked this pull request as ready for review January 29, 2024 15:46
@frcroth frcroth requested a review from fm3 January 29, 2024 15:46
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Looking good :)

Do you think it would help to introduce a trait LayerWithMagLocators that implements a withMags method (and does the switch case over the different implementations there?). That way we wouldn’t have to do all of this switch-casing everywhere in the business logic? Did not test if this would work but maybe it’s worth trying.

Also, thanks for going through the docs! 👏

layer.copy(mags = layer.mags.map(m => m.copy(path = m.path.map(_.split("/").takeRight(2).mkString("/")))))

private def makeZarrSegmentationLayerPathRelative(layer: ZarrSegmentationLayer): DataLayer =
layer.copy(mags = layer.mags.map(m => m.copy(path = m.path.map(_.split("/").takeRight(2).mkString("/")))))
Copy link
Member

Choose a reason for hiding this comment

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

what is this doing? 🤔

Also, should probably be extracted to makeMagPathRelative since this is used twice here.

For N5/precomputed this is not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is used for zarr because of the different directory format (for ngff and zarr, there is an additional directory e.g. color)

@frcroth
Copy link
Member Author

frcroth commented Feb 5, 2024

Looking good :)

Do you think it would help to introduce a trait LayerWithMagLocators that implements a withMags method (and does the switch case over the different implementations there?). That way we wouldn’t have to do all of this switch-casing everywhere in the business logic? Did not test if this would work but maybe it’s worth trying.

Have a look now. I think it does make sense, but I am not sure I like the name. Is there something more abstract that seperates layers with maglocators from others?

@fm3
Copy link
Member

fm3 commented Feb 5, 2024

Have a look now. I think it does make sense, but I am not sure I like the name. Is there something more abstract that seperates layers with maglocators from others?

Good question! I’m afraid, not at this point. Layers without magLocators are currently

  • annotation layers
  • dataset layers with wkw format

In fact, I think we should add magLocator support for wkw layers as well, either with a migration of the jsons or an adapter that can read WKWResolution objects and turn them into MagLocators. This would also enable remote wkw datasets. But that does not have a high priority atm and it should not block this PR.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

🎉

@frcroth frcroth merged commit 06ffdb1 into master Feb 5, 2024
2 checks passed
@frcroth frcroth deleted the upload-more-datatypes branch February 5, 2024 13:47
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.

Support uploading n5 and precomputed datasets
2 participants