-
Notifications
You must be signed in to change notification settings - Fork 12
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
Reading nd datasets #966
Reading nd datasets #966
Conversation
…ween old BoundingBoxes and new NDBoundingBoxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first round of review with some suggestions and many questions.
@@ -60,6 +70,9 @@ class ArrayInfo: | |||
voxel_type: np.dtype | |||
chunk_shape: Vec3Int | |||
chunks_per_shard: Vec3Int | |||
shape: VecInt = VecInt(1, 1, 1, 1) | |||
dimension_names: Tuple[str, ...] = ("c", "x", "y", "z") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is called dimension_...
instead of axis_...
? Does it make sense to use a different terminology herre?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axes_... is used in the json, webknossos and the libs so far, dimension_... is terminology of the Zarrita Array create method.
chunk_bbox.get_3d("topleft") - bbox.get_3d("topleft") | ||
) | ||
section_bottomright = Vec3Int( | ||
chunk_bbox.get_3d("bottomright") - bbox.get_3d("topleft") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be ... - bbox.get_3d("bottomright")
? or is this an offset in which case chunk_bbox.get_3d("bottomright").offset(...)
might make this more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an offset. But the Vec3Int class currently doesn't have an offset method implemented. I could apply the offset to the chunk_bbox but, as I need to modify both Vec3Ints again, after adding the offset, I think the readability wouldn't be much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, impressive stuff 🥇 kudos for towing this towards the finish line. I left a couple of comments and several questions.
the biggest impact would probably be to add the channel axis to the ND bbox. I don't know whether there was a discussion which decided against this? or is it only because WK doesn't list the channel as an additional coordinate? I think, it could simplify a lot of code if the channel was included in the nd bbox, but it's probably also quite some work to do so...
Overall, I'd like to see a bit more documention:
- a small doc section which explains how ND support works. for example, I still don't have a super good grasp of the axes order stuff.
- are there breaking changes for users of wklibs? e.g., topleft -> topleft_xyz? this would be quite important to know.
Also, maybe you could add a few more sentences or bullet points to the PR description to explain what was changed in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this merged and released so we can startet dog fooding these changes.
Description:
Issues:
Todos:
Make sure to delete unnecessary points or to check all before merging: