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

fix(array): thread order parameter through to Array.__init__ #2417

Merged
merged 9 commits into from
Oct 21, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Oct 19, 2024

The goal here is to allow configuring the runtime memory order of arrays. We had all the pieces in place but we stopped short of passing order through to Array.__init__.

closes #2408

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@jhamman jhamman requested a review from d-v-b October 19, 2024 20:56
@@ -611,7 +609,7 @@ async def _create_v2(
if not exists_ok:
await ensure_no_existing_node(store_path, zarr_format=2)
if order is None:
order = "C"
order = config.get("array.order", "C")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose we change the key array.order to array_indexing.order in the config, this will happily return "C", when the correct behavior should be an error. Therefore I think we should define the default value higher up than this function, and this function should have a hard dependency on the schema of the config.

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 not sure I follow. What's the ask here?

Copy link
Contributor

@d-v-b d-v-b Oct 21, 2024

Choose a reason for hiding this comment

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

I think the "default array order is C" logic should be defined higher up in the stack, instead of in this function. so the change would be to a) do that, and b) make this line order = config.get("array.order"), i.e. something that will error if we change the schema of the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

How much higher? We could require passing order into Array.create rather than allowing this to be a nullable field. Or we could do the config check there.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to put the default in the config? I'm guessing we will have a lot of defaults (e.g., codecs), so would probably make sense to put them all in the same place

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the defaults are already in the config, in which case I think we can just remove any "default to C" behavior in _create_v2

Copy link
Member Author

Choose a reason for hiding this comment

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

okay. I think the final state should be what you are after. Because the v2 array metadata requires order, we do have to check the config setting if no order was passed, but we no longer apply a default on top of that.

tests/test_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@d-v-b d-v-b left a comment

Choose a reason for hiding this comment

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

lgtm!

@jhamman jhamman merged commit 4a18c5a into zarr-developers:main Oct 21, 2024
26 checks passed
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.

[v3] zarr.array silently ignores order kwarg
2 participants