fix: check for non-int 0 fill values

[ilan-gold]

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)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (205f5f8) to head (5d71e9f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3345   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files          89       89           
  Lines       11901    11902    +1     
=======================================
+ Hits        11132    11133    +1     
  Misses        769      769           

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]

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

[dstansby]

Looks good to me - needs a release note entry, and I left one suggestion to improve the test too.

[dstansby]

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?

[ilan-gold]

I think everything is cast anyway by dtype, so it shouldn't matter, no?

[ilan-gold]

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.

[ilan-gold]

Ok apologies, I know that Tom's PR supercedes this in many ways but for anyone curious, my worry was about how memory is allocated.

I learned that zeros is only faster than full because it doesn't allocate memory when requested, lazily doing so later when needed (see https://stackoverflow.com/questions/70055063/how-is-memory-handled-once-touched-for-the-first-time-in-numpy-zeros/70056188#70056188). In benchmarks, I see the times of subsequent operations on the matrix, whether something like sum that should touch all elements or __setitem__ to be identical independent of initialization so I definitely think this should be merged!

import numpy as np
buf = np.arange(100_000, dtype=np.float64)
# cell 1
%%timeit full = np.full((1_000_000,), 0, dtype=np.float64)

full[900_000:] = buf
# cell 2
%%timeit zeros = np.zeros((1_000_000,), dtype=np.float64)
    
zeros[900_000:] = buf
# cell 3
%%timeit full = np.full((1_000_000,), 0, dtype=np.float64)

full.sum()
# cell 4
%%timeit zeros = np.zeros((1_000_000,), dtype=np.float64)

zeros.sum()

[d-v-b]

@ilan-gold is this still active?

[ilan-gold]

Looking back at the history here, the PR itself, and my last comment, I don't see why not. It's pretty marginal though. Maybe @TomAugspurger has an opinion.