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

[v3] Array.append #2413

Merged
merged 7 commits into from
Oct 23, 2024
Merged

[v3] Array.append #2413

merged 7 commits into from
Oct 23, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Oct 19, 2024

Ports the Array.append method over from 2.18 (plus tests for resize/append). Mostly a copy-and-paste but does come with a bit of extra behavior changes:

  • Array class is now a non-Frozen dataclass. This is needed to support property setters. I've added a note describing when we can go back to frozen=True
  • Array.resize is an in-place op again. As we discussed at the meeting yesterday, it really didn't make sense to have this any other way (what would you do with the old array object?)

closes #2404

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • GitHub Actions have all passed

changes the Array.resize to be an inplace operation
return replace(self, metadata=new_metadata)

# Update metadata (in place)
object.__setattr__(self, "metadata", new_metadata)
Copy link
Member Author

Choose a reason for hiding this comment

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

@TomAugspurger - calling this out because it will impact the xarray use of resize... now in-place again :)

src/zarr/core/array.py Outdated Show resolved Hide resolved
Comment on lines +422 to +424
@pytest.mark.parametrize("store", ["memory"], indirect=True)
@pytest.mark.parametrize("zarr_format", [2, 3])
def test_resize_1d(store: MemoryStore, zarr_format: int) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

to fight test bloat we should find a way to parameterize over dimensionality instead of making separate 1d, 2d, etc tests. but that's not a blocker here.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree! These tests were copied over from v2 so I'm going to leave them for now.

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.

looks good! I think we just need a more informative exception here.

@jhamman jhamman requested a review from d-v-b October 21, 2024 03:14
@jhamman
Copy link
Member Author

jhamman commented Oct 23, 2024

pre-commit.ci autofix

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 6ce0526 into zarr-developers:main Oct 23, 2024
28 checks passed
@jhamman jhamman deleted the feature/append-v3 branch October 23, 2024 15:37
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] AttributeError: 'Array' object has no attribute 'append'
2 participants