BUG: Reset an inherited fill_value in MaskedArray.__new__ as well by danggeun · Pull Request #32562 · numpy/numpy

danggeun · 2026-09-09T21:22:04Z

Follow-up to #32508, as suggested there by @ngoldbaum.

MaskedArray.__new__ re-checks the fill_value inherited from data without the fallback that #32423 and #32508 added to _update_from, so an out-of-range value was still kept on this path:

>>> a = np.ma.arange(9.0)
>>> a.fill_value
np.float64(1e+20)
>>> np.ma.array(a, dtype='int64').fill_value
RuntimeWarning: invalid value encountered in cast
np.int64(9223372036854775807)

The fallback now lives in one helper, _check_fill_value_or_default, used by both _update_from and __new__. In __new__ it applies only to the fill_value inherited from data; a fill_value passed by the caller still goes through _check_fill_value unchanged and raises as before.

I checked the other _check_fill_value call sites: the remaining ones either take a fill_value given by the user, where raising is correct, or run after _update_from has already normalised it. This was the last path where an inherited fill_value could survive a dtype it cannot represent.

The release notes for #32423, #32508 and this PR are made identical so that towncrier groups them into one entry.

Testing: one case added next to the ones from #32508, covering the constructor path (no warning, default fill_value). The rest of the numpy/ma suite passes unchanged.

AI Disclosure

I used an AI assistant (Claude) for the patch prototype, for checking the other call sites, and for translation and grammar editing of this text. I ran the reproduction and the numpy/ma suite before and after myself.

Follow-up to numpy#32508.

jorenham · 2026-09-09T21:36:33Z

existing release notes shouldn't be edited

@ngoldbaum suggested this in #32508: making the content identical so that towncrier groups the entries into one. Happy to revert the two existing fragments and keep only the new one if you'd rather they stay untouched.

@jdavies-st do you mind the combined release note for all three PRs?

Yes, no problem. That makes a lot of sense, as these 3 PRs are an expanding circle of fixes for the same problem.

jorenham · 2026-09-09T21:36:39Z

I asked them to do that. It’s fine to edit release notes IMO although we should ping the original author too.

ngoldbaum

Thanks for the followup! I have some feedback below.

ngoldbaum · 2026-09-10T19:07:08Z

+    """
+    Like `_check_fill_value`, but return None when `fill_value` cannot be
+    represented in `ndtype`, so the caller falls back to the default.
+
+    Used where a fill_value is inherited from another array rather than
+    given by the user.  A user-given fill_value must still raise.  A float
+    that overflows an integer dtype does not raise in `_check_fill_value`;
+    it only sets the floating point invalid flag, so that is raised here.
+
+    """


this private helper doesn't need a docstring IMO

ngoldbaum · 2026-09-10T19:08:02Z

+                fill_value = _check_fill_value_or_default(fill_value,
+                                                          _data.dtype)


Suggested change

fill_value = _check_fill_value_or_default(fill_value,

_data.dtype)

fill_value = _check_fill_value_or_default(

fill_value, _data.dtype)

This is a more readable way to format arguments if you need to break it up onto two lines

ngoldbaum · 2026-09-10T19:08:40Z

        if fill_value is None:
            fill_value = getattr(data, '_fill_value', None)
+            if fill_value is not None:
+                # Inherited from `data`, which may have a different dtype.


IMO this comment isn't needed either because it's obvious from two lines above this came from data. When working on any codebase with an LLM, always be skeptical of whether or not a comment it adds is actually useful to a human or just the LLM thinking out loud in its context window.

ngoldbaum · 2026-09-10T19:20:46Z

        # Update fill_value.
        if fill_value is None:
            fill_value = getattr(data, '_fill_value', None)
+            if fill_value is not None:
+                # Inherited from `data`, which may have a different dtype.
+                fill_value = _check_fill_value_or_default(fill_value,
+                                                          _data.dtype)
        # But don't run the check unless we have something to check.
        if fill_value is not None:
            _data._fill_value = _check_fill_value(fill_value, _data.dtype)


Your PR sets the errstate and calls _check_fill_value twice on every ma.array/ma.asarray from a masked array with a materialized fill_value, even when the dtype doesn't change.

How about this instead?

Suggested change

# Update fill_value.

if fill_value is None:

fill_value = getattr(data, '_fill_value', None)

if fill_value is not None:

# Inherited from `data`, which may have a different dtype.

fill_value = _check_fill_value_or_default(fill_value,

_data.dtype)

# But don't run the check unless we have something to check.

if fill_value is not None:

_data._fill_value = _check_fill_value(fill_value, _data.dtype)

# Update fill_value.

if fill_value is not None:

_data._fill_value = _check_fill_value(fill_value, _data.dtype)

else:

fill_value = getattr(data, '_fill_value', None)

if fill_value is not None:

if getattr(data, 'dtype', None) != _data.dtype:

fill_value = _check_fill_value_or_default(fill_value,

_data.dtype)

if fill_value is not None:

_data._fill_value = fill_value

else:

_data._fill_value = _check_fill_value(fill_value,

_data.dtype)

Applied, thanks.

ngoldbaum · 2026-09-10T19:24:04Z

+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            b = array(a, dtype="int64")
+        assert_equal(b.fill_value, default_fill_value(b.dtype))


Could you add two more cases here? A user-supplied fill_value=1e20 in the constructor should still raise TypeError, and an object array with the default '?' fill passed with dtype=float now gets the default instead of raising.

Added both. The object one reads a.fill_value first, otherwise there's nothing to inherit.

ngoldbaum

Thanks @danggeun!

BUG: Reset an inherited fill_value in MaskedArray.__new__ as well

a79fbe1

Follow-up to numpy#32508.

github-actions Bot added the 00 - Bug label Sep 9, 2026

DOC: name release note for numpygh-32562

d836475

jorenham reviewed Sep 9, 2026

View reviewed changes

charris added the 09 - Backport-Candidate PRs tagged should be backported label Sep 10, 2026

ngoldbaum requested changes Sep 10, 2026

View reviewed changes

ngoldbaum reviewed Sep 10, 2026

View reviewed changes

MAINT: address review on numpygh-32562

00c17ef

danggeun requested a review from ngoldbaum September 16, 2026 21:04

ngoldbaum approved these changes Sep 16, 2026

View reviewed changes

ngoldbaum merged commit 54a307a into numpy:main Sep 16, 2026
91 checks passed

danggeun deleted the bugfix/ma-fill-value-new branch September 16, 2026 23:50

		fill_value = _check_fill_value_or_default(fill_value,
		_data.dtype)

Uh oh!

Conversation

danggeun commented Sep 9, 2026

AI Disclosure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants