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

Single startswith() call instead of multiple ones #1556

Merged
merged 3 commits into from Dec 7, 2023

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Oct 31, 2023

It's faster and probably more readable.

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)

It's faster and probably more readable.
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Oct 31, 2023
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review October 31, 2023 10:32
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Dec 7, 2023
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Thanks @DimitriPapadopoulos.

(fyi, this code path will be leaving Zarr soon but no harm in merging for now)

@jhamman jhamman enabled auto-merge (squash) December 7, 2023 21:27
@jhamman jhamman merged commit 40a6e81 into zarr-developers:main Dec 7, 2023
21 checks passed
@DimitriPapadopoulos DimitriPapadopoulos deleted the startswith branch December 7, 2023 22:26
@jakirkham
Copy link
Member

Wonder if this is part of performance degradation with v3 that was called out before

cc @rabernat (for awareness)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Dec 7, 2023

Replacing two function calls with a single function call should be faster, unless key.startswith("data/") is usually true. In that case, the second function call key.startswith("meta/") would seldom be triggered. But this would actually happen only when a ValueError is raised, which shouldn't happen too often and is expensive in itself.

jhamman added a commit to jhamman/zarr-python that referenced this pull request Jan 24, 2024
It's faster and probably more readable.

Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
Co-authored-by: Joe Hamman <joe@earthmover.io>
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.

None yet

4 participants