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

Explore remote N5 datasets #6520

Merged
merged 8 commits into from
Oct 6, 2022
Merged

Explore remote N5 datasets #6520

merged 8 commits into from
Oct 6, 2022

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Sep 30, 2022 •
edited
Loading

Note for reviewer: git didn’t catch the renaming of ExploreRemoteLayerService. When checking the diff, note that many of its functions have been moved to other files, but not changed

URL of deployed dev instance (used for testing):

Steps to test:

  • click add dataset in dataset dashboard tab
  • paste uri to n5 dataset layers
  • valid datasource-properties.json should be returned

Issues:

@fm3 fm3 self-assigned this Sep 30, 2022
@fm3 fm3 marked this pull request as ready for review October 4, 2022 08:48
@fm3 fm3 changed the title [WIP] Explore remote N5 datasets Explore remote N5 datasets Oct 4, 2022
@fm3
Copy link
Member Author

fm3 commented Oct 4, 2022

@philippotto if you have time, maybe you could adapt the front-end to reflect the change that remote datasets can now also be n5 instead of zarr (not sure how prominently we want to promote this feature).
I’d say that the backend part could also be merged as is, so the frontend changes do not necessarily block this PR

@fm3 fm3 requested a review from jstriebel October 4, 2022 10:03
jstriebel
jstriebel approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

LGTM, love the new Explorer hierarchy ❤️

app/models/binary/explore/N5MultiscalesExplorer.scala Outdated
Comment on lines 70 to 81
case "" => Fox.successful(1.0)
case "ym" => Fox.successful(1e-15)
case "zm" => Fox.successful(1e-12)
case "am" => Fox.successful(1e-9)
case "fm" => Fox.successful(1e-6)
case "pm" => Fox.successful(1e-3)
case "nm" => Fox.successful(1.0)
case "µm" => Fox.successful(1e3)
case "mm" => Fox.successful(1e6)
case "cm" => Fox.successful(1e7)
case "dm" => Fox.successful(1e8)
case "m" => Fox.successful(1e9)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this can be shared with the other unit definitions? Or are those too different?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I’ve seen, ngff uses nanometer while n5 uses nm so they are pretty distinct. Of course, we could still merge the matching into one big function, what would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it separate as-is for now 👍
The only reference I found is https://github.com/saalfeldlab/n5-viewer/blob/23f86b32e77ff1856893fa955523fb016cef3d82/README.md#container-structure,
where it says that not setting anything assumes µm, I think that is currently not the case. Also, it specifically shows the string um, which is missing in this list.

@fm3 fm3 mentioned this pull request Oct 6, 2022
Merged
2 tasks
@fm3 fm3 merged commit ae72df6 into master Oct 6, 2022
@fm3 fm3 deleted the explore-n5 branch October 6, 2022 13:46
hotzenklotz added a commit that referenced this pull request Oct 13, 2022
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into pro…
8810958 
…jects-created

* 'master' of github.com:scalableminds/webknossos: (337 commits)
  Fix docs for the annotation download file format (#6546)
  Added total runtime information to VX reports (#6543)
  fix VX report for completed + skipped tasks (#6540)
  Avoid allocating spire uint objects during apply agglomerate (#6532)
  Explore remote N5 datasets (#6520)
  Fix MeshChunk byteOffset (Long, not Int) (#6536)
  update browserslist (#6505)
  Support new Mesh File (v3) (#6491)
  makes workflow_yamlContent optional (#6518)
  Always return 404 for Failures in Zarr Streaming (#6515)
  Poll wk version to notify during upgrade (#6451)
  add script which extracts newest changelog and creates GH release for it (#6504)
  release 22.10.0 (#6500)
  voxel³ -> voxel (#6501)
  Allow task type summary to identify task type when creating tasks in bulk (#6486)
  Fix sql evolution 090 (defer not null constraint) (#6498)
  SQL schema cleanup (#6492)
  Fix validation of layer selection when trying to start globalization of floodfills (#6497)
  Add "shift + w" shortcut to cycle backwards through tools (#6493)
  Fix filtering for public datasets in dataset table (#6496)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstriebel jstriebel jstriebel approved these changes

Assignees

@fm3 fm3

Labels
backend frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Explore/Import remote N5 datasets
2 participants
@fm3 @jstriebel