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 data loading with other axis orders #7592

Merged
merged 16 commits into from
Feb 5, 2024
Merged

ND data loading with other axis orders #7592

merged 16 commits into from
Feb 5, 2024

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jan 29, 2024

  • enables ND data loading for all data formats with AxiOrder and AdditionalCoordinates (precomputed, n5, zarr3) (previously only zarr2)
  • volume annotation data.zips should now be viewable as datasets after unpacking (fixed the indices of their additional coordinates)
  • unifies DatasetArrayHandle (previously CubeHandle, no longer a trait with multiple implementations)
  • no more distinction between reading with or without AdditionalCoordinates from DatasetArray
  • Added some more tryos for better error handling
  • FullAxisOrder is now constructed for reading with arbitrary ND axis orders, with permutations from array order to wk order (tcxyz), including one for flipping c to fortran in the process
  • Adds some tests for these permutatinos

Steps to test:

  • Unpack zarr volume download data zip (both 3d and 4d), should be viewable as dataset
  • View 4d and 3d and 2d zarr datasets, should be viewable

TODOs:

  • fix volume annotation metadata (additional axes have wrong indices)
  • fix permute (needs to take additional axes into account)
  • test with volume annotations
  • test with 2d zarr
  • add tests for the permutations in c and f
  • fix viewing nd datasets without the additional coordinates in the json (cxyz should still be accessible if normal axisOrder field is correct)

Issues:


  • Updated changelog
  • Needs datastore update after deployment

@fm3 fm3 self-assigned this Jan 29, 2024
@fm3 fm3 marked this pull request as ready for review January 31, 2024 14:49
@fm3 fm3 changed the title WIP: ND data loading with other axis orders ND data loading with other axis orders Jan 31, 2024
@fm3 fm3 requested a review from frcroth January 31, 2024 15:25
Copy link
Member

@frcroth frcroth left a comment

Choose a reason for hiding this comment

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

Very good job!

val chunkIndices = ChunkUtils.computeChunkIndices(datasetShape.map(axisOrder.permuteIndicesReverse),
axisOrder.permuteIndicesReverse(chunkShape),
val chunkIndices = ChunkUtils.computeChunkIndices(datasetShape.map(fullAxisOrder.permuteIndicesArrayToWk),
fullAxisOrder.permuteIndicesArrayToWk(chunkShape),
shape,
totalOffset)
if (partialCopyingIsNotNeeded(shape, totalOffset, chunkIndices)) {
for {
chunkIndex <- chunkIndices.headOption.toFox
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
chunkIndex <- chunkIndices.headOption.toFox
chunkIndexInWk <- chunkIndices.headOption.toFox

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m hesitant, because then we would have to annotate shape, offset, chunkIndices, sourceChunk as well. It’s true that this ordering is important to understanding this function, but it is not the only thing going on here, so I’m scared the renamings would generate too much noise. Instead, I tried to make the comment more descriptive. What do you think?

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 fine, either way this PR should be an improvement.

@fm3 fm3 merged commit a979ffc into master Feb 5, 2024
2 checks passed
@fm3 fm3 deleted the axis-order-nd branch February 5, 2024 15:12
@fm3 fm3 mentioned this pull request Feb 6, 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.

Support ND data with arbitrary axis orders
2 participants