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

ND Datasets #7136

Merged
merged 148 commits into from
Sep 5, 2023
Merged

ND Datasets #7136

merged 148 commits into from
Sep 5, 2023

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Jun 13, 2023

This PR adds support for n-d datasets and annotations. Such datasets can be imported by using zarr.

See #7229 for restrictions and follow-up tasks regarding n-d support.

URL of deployed dev instance (used for testing):

Steps to test:

  • go to https://d.webknossos.xyz/datasets/sample_organization/idr0101A-4d/view#1114,1075,17,0,0.977
    • was imported remotely with https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0101A/13457227.zarr
  • use the t-slider in the toolbar to switch between different t values
  • create a new annotation
  • create skeleton nodes at different t values
    • edges should be half-transparent in that case
  • create segments at different t values
  • reload the page
  • everything should be at the correct t values
  • download/re-import skeletons via the skeleton tab
  • download/re-upload whole annotation (not sure whether this is implemented yet?)

Issues:

Todo:

  • Backend
    • accept additionalCoords as property in update actions
      • skeletons. e.g., for createNode { "name": "createNode", "value": { "position": [...], "additionalCoords": [{ "name": "t", "value": 10 }], ...}}
      • volume. e.g., updateBucket and createSegment
        • Save these in FossilDB. This requires changing Morton code calculation in https://github.com/scalableminds/webknossos-wrap ? Or only store xyz in morton code and additional coords afterward ✔️
          • Morton code is implemented currently. However, with only 64 Bits available, the coordinate bounds are very limited for high dimensions: Maximum coordinate = 2^(64/Dimensions) so with 5 dimensions already maximum coordinate value is 4096.
          • Validate that storing and getting them back actually works (did some weird regex parsing there)
          • Export/import for volume downloads/uploads
      • allow to store additional coords in tracing (next to editPosition)
    • TracingStore: Move AdditionalCoordinateRequest.toProto to ProtoGeometryImplicits? Caused weird compiler errors at different places previously, maybe investigate
    • Update DatasetArray
    • Infer bounds of additional coordinates
    • Make a clearer distinction between additionalCoordinate[request vs definition]. Maybe introduce new names?
    • Import/export of nd-skeletons in NML (nodes should get properties, such as additionalCoordinate-t="10")
    • NML import or export of Volume annotations does not seem to work yet -> change wkw
    • consider writing tests (Added 13 tests 😮 )
    • On merging annotations / tracings etc., merge the additional coords
    • Fix volume tracing bucket keys not needing migrations
  • frontend
    • adapt data loading and rendering to higher dimensions
    • adapt skeletons to higher dimensions
    • test saving after backend was adapted
    • encode additional coords in url
    • respect in NML export
    • tackle remaining todo comments
    • bugs
      • fix rendering of skeletons
    • fix tests
    • fix cyclic dependencies

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

philippotto and others added 22 commits June 12, 2023 14:50
@frcroth
Copy link
Member

frcroth commented Aug 21, 2023

@philippotto After consultation with @fm3 I have changed backend AdditionalCoordinateRequest to AdditionalCoordinate and AdditionalCoordinateDefinition to AdditionalAxis. To reflect this, Datalayers now longer have an additionalCoordinates key, but an additionalAxes key (also in the JSON). Can you change the frontend to reflect this naming (axis for the definition)? (e.g. in nml and everything working with backend jsons)

@philippotto
Copy link
Member Author

@philippotto After consultation with @fm3 I have changed backend AdditionalCoordinateRequest to AdditionalCoordinate and AdditionalCoordinateDefinition to AdditionalAxis. To reflect this, Datalayers now longer have an additionalCoordinates key, but an additionalAxes key (also in the JSON). Can you change the frontend to reflect this naming (axis for the definition)? (e.g. in nml and everything working with backend jsons)

Done. FYI: During testing, the bucket requests took "forever" (at some point, the requests time out then). With a bit of patience, some requests succeeded then (a single request took 20 seconds, though). Not sure whether we can do anything about that..

@frcroth frcroth requested a review from fm3 August 23, 2023 08:20
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.

Getting close :) I added two more comments on the backend code.

Maybe you could look into the slow requests philipp mentioned. Maybe this is just due to how the test dataset is stored, but maybe we perform more requests than really needed?

Also, could you have another look at the checkboxes in my comment above? I think some of that is already resolved in the meantime.

@frcroth
Copy link
Member

frcroth commented Sep 4, 2023

@philippotto

Done. FYI: During testing, the bucket requests took "forever" (at some point, the requests time out then). With a bit of patience, some requests succeeded then (a single request took 20 seconds, though). Not sure whether we can do anything about that..

Do you mean automatic testing or manually looking at the dataset and changin t/browsing around? Manual testing works very well for me with the local dataset.

@frcroth frcroth requested a review from fm3 September 4, 2023 08:51
@fm3
Copy link
Member

fm3 commented Sep 4, 2023

I just went and did a bit more testing and found a bug:

  • Volume Download + Reupload seems to be broken. I brushed some in a fresh annotation, hit download, and then uploaded the same annotation via drag+drop of the zip to the dashboard. The resulting annotation did not have any visible brushed segmentation. This still works on master. @frcroth could you have a look at what might be going on there?

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.

Let’s go :shipit:

@philippotto philippotto merged commit 978dde5 into master Sep 5, 2023
1 check passed
@philippotto philippotto mentioned this pull request Sep 5, 2023
3 tasks
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.

Add timeseries support (4D)
5 participants