Skip to content

fix: check for non-int 0 fill values #3345

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Aug 5, 2025

A bit of an "oops" from me, but hopefully this is more robust (than #3082)! It worked for my one example but testing this without using a spy object seems impossible (happy to contribute that though, or some other test).

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/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@ilan-gold ilan-gold changed the title fix: check for non-int fill values fix: check for non-int 0 fill values Aug 5, 2025
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 5, 2025
Copy link

codecov bot commented Aug 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.54%. Comparing base (a26926c) to head (8c3fa27).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3345   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files          78       78           
  Lines        9423     9424    +1     
=======================================
+ Hits         8909     8910    +1     
  Misses        514      514           
Files with missing lines Coverage Δ
src/zarr/core/buffer/cpu.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
Copy link
Contributor

d-v-b commented Aug 6, 2025

to test this, could we perhaps patch np.full and np.zeros to raise exceptions, that you would then catch in the test? I'm thinking there would be 2 test functions, 1 testing the various inputs that should hit the np.full branch, and another testing the various inputs that should hit the np.zeros branch. This could also work for a single test function that internally checks which numpy routine the input scalar should trigger

Copy link
Contributor

@dstansby dstansby 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 - needs a release note entry, and I left one suggestion to improve the test too.



@pytest.mark.parametrize("dtype", [np.int8, np.uint16, np.float32, int, float])
@pytest.mark.parametrize("fill_value", [None, 0, 1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@pytest.mark.parametrize("fill_value", [None, 0, 1])
@pytest.mark.parametrize("fill_value", [None, 0, 0.0, 1])

Worth explicitly including a float here?

@dstansby dstansby added this to the 3.1.2 milestone Aug 11, 2025
@ilan-gold
Copy link
Contributor Author

I've done some digging into this topic, I'll post in a bit but I would like to hold off on merging for a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants