Skip to content

Conversation

calvinchai
Copy link
Contributor

No description provided.

@calvinchai
Copy link
Contributor Author

@balbasty I've done the suggested fix. Please take a look and see if there is anything needs attention. I would hold the merge for a moment. I want to include a few more improvement and fixes including nifti header integration. I will post a issue related to that.

@balbasty
Copy link
Collaborator

balbasty commented Aug 8, 2025

Before anything, I thought one of the conclusions of the previous discussion was that it would be useful to have the package works when TS is available, but not ZP. Now you've made the TS driver depend on the ZP driver, which makes this feature impossible to implement.

I still think it's a good idea to have both drivers completely independent and be able to pick only one of them as a dependency.

@calvinchai
Copy link
Contributor Author

Before anything, I thought one of the conclusions of the previous discussion was that it would be useful to have the package works when TS is available, but not ZP. Now you've made the TS driver depend on the ZP driver, which makes this feature impossible to implement.

I still think it's a good idea to have both drivers completely independent and be able to pick only one of them as a dependency.

I've reverted the changes and added the required feature. Now the tensorstore driver can operate without zarr-python at all.

@balbasty
Copy link
Collaborator

Fantastic @calvinchai ! Great effort
I don't have more comments on the new code, looks good.

Do we need a few new tests? Like maybe writing a sharded zarr with tensorstore when zarr-python is unavailable?

@calvinchai
Copy link
Contributor Author

Fantastic @calvinchai ! Great effort I don't have more comments on the new code, looks good.

Do we need a few new tests? Like maybe writing a sharded zarr with tensorstore when zarr-python is unavailable?

Sure. Currently the test case will cover using both drivers, if we want to test this scenario we need a separate env. I will see how can we do this with github CI.

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.

5 participants