Skip to content

Conversation

@ehennestad
Copy link

@ehennestad ehennestad commented Sep 29, 2025

Fix issue #120

Add a check in ZarrPy/readZarrfor whether starts, ends or strides are integers, and wrap them in a list if true.

Some alternatives:

  • wrap in array to ensure type is same as what is received from MATLAB when these are non-scalar?
  • simplify check and assume all is scalar if one is scalar

Simplify - Expecting input to be array or int, never None
@krisfed
Copy link
Member

krisfed commented Oct 6, 2025

Thank you for contributing!!! Looks reasonable to me.

Looks like we don't have workflow enabling tests to run for PRs from forks, let me fix that.

I was going to ask to add a test, but it seems currently there is no way to write a scalar dataset (with shape that is a scalar value) using this project (Is that a significant limitation?). For now, if you are ok adding starting_time or a different sample scalar dataset to test/dataFiles and adding a simple test in test/tZarrRead.m to read it, that would be great! If not, you can add a task for us to add a test later.

@krisfed krisfed requested review from jhughes-mw and krisfed October 6, 2025 16:55
@krisfed
Copy link
Member

krisfed commented Oct 6, 2025

Even if you are not able to add a test, could you please do a dummy commit to trigger the tests workflow on the PR? Thanks!

@ehennestad
Copy link
Author

I can add a test. Being able to write scalar datasets is out of scope at the moment, we will first work against full support for read

@krisfed
Copy link
Member

krisfed commented Oct 8, 2025

Sounds good, thanks!

Yes, sorry, didn't meant to imply you have to work on writing scalar datasets, just thinking if that's something we should look into in the future.

Copy link
Member

@krisfed krisfed left a comment

Choose a reason for hiding this comment

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

Looks good to me!

As mentioned - would be nice to have a test though, and to make sure existing tests run fine.

Thanks for contributing!

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.

2 participants