Skip to content

Conversation

@srivarra
Copy link
Collaborator

@srivarra srivarra commented Oct 28, 2025

Adds Position.compute_pyramids

@srivarra srivarra linked an issue Oct 28, 2025 that may be closed by this pull request
@srivarra srivarra marked this pull request as ready for review October 28, 2025 16:40
@srivarra srivarra requested a review from ieivanov October 28, 2025 16:40
@srivarra srivarra requested a review from ieivanov November 15, 2025 00:42
@ieivanov
Copy link
Contributor

Sorry if I'm being a pain in the neck during this review, I think iohub follows higher standards than some of our other code. I hope my requests make sense.

@srivarra
Copy link
Collaborator Author

srivarra commented Nov 22, 2025

Sorry if I'm being a pain in the neck during this review, I think iohub follows higher standards than some of our other code. I hope my requests make sense.

No I prefer this haha, I'm learning a lot from you so I think it's a good thing tbh.

@srivarra srivarra linked an issue Dec 1, 2025 that may be closed by this pull request
@srivarra srivarra mentioned this pull request Dec 2, 2025
@srivarra srivarra requested a review from ieivanov December 2, 2025 19:13
@ieivanov
Copy link
Contributor

I think we are very close! I'm not fully done reviewing the PR, but before I forget - it looks like _delete_pyramids deletes the zarr entries, but does not modify the ome metadata. Please extend _delete_pyramids such that it leaves a proper ome zarr store, even if you don't decide to recreate the pyramid levels

@srivarra
Copy link
Collaborator Author

@ieivanov I had to make some adjustments due to 972aea1 (#348). Ready for re-review.

@ieivanov
Copy link
Contributor

I think this PR looks good! Sorry, I'll change my mind on one thing - we should rename _initialize_pyramid back to initialize_pyramid. This way we are not introducing a breaking API change in this PR. Plus, the method may be useful if folks want to use iohub to initialize the pyramid structure and create ome metadata, but then compute them some other way.

Before we merge I'd love to get a review from @JoOkuma.

We should also refactor czbiohub-sf/biahub#12 to use these latest changes - this will tell us if we've gotten the API right.

@ieivanov ieivanov requested a review from JoOkuma December 18, 2025 03:26
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.

iohub convert fails on dataset with size (261, 1332, 2000) Compute Pyramids for Images

3 participants