gh-104263: Rely on Py_NAN and introduce Py_INFINITY

[seberg]

It seems to me code all around relies on both Py_NAN and Py_HUGE_VAL being correct anyway. The actual value for Py_NAN is subtly incorrect on MIPS (depending on settings) or at least nonstandard, which seems to confuse some builtin functions.
(Probably it is signalling, but NumPy saw this with fmin, which probably should also ignore signalling NaNs, see also numpy/numpy#23158).

The guards about _PY_SHORT_FLOAT_REPR making sense are relatively unrelated to NAN and INF being available.

Nevertheless, I currently hide the Py_NAN definition if that is not set, since I am not sure what good alternative there is to be certain that Py_NAN is well defined.
OTOH, I do suspect there is no platform where it is not and it should probably be changed?!

---

@stefanor reported that this NAN value creates odd issues with NaN (or system fmin) for NumPy. I don't have MIPS, and don't plan to confirm that this fixes the issue (even if it is a bit of taping over).

For me the main point is that it deletes a bunch of code that seems unnecessary.

• Issue: IEEE floating point signalling bit can be inverted on MIPS (Python assumes its not) #104263

[JelleZijlstra]

I'm not familiar with floating-point behavior, but from the linked numpy issue it seems there is an arguable, obscure CPython bug. Could you open an issue so we can discuss whether CPython needs to change, and add tests for the behavior that we should want?

[mdickinson]

Hmm. There's some history here. The avoidance of Py_NaN and Py_HUGE_VAL was deliberate, at least in part to avoid operations that might set floating-point exceptions.

I don't think there's any need to change the infinity-handling code here; that bit pattern should be correct regardless of machine; could we stick to just looking at the NaN operations?

One other thing: Py_NaN will not necessarily have the same sign as the value computed from the bit pattern, and I'd expect people to notice and complain if the sign bit changes (arguably they shouldn't, but that's a different discussion). I'd prefer to be in control of the sign (and the payload) here, and the Py_NaN macro doesn't give us that control.

Instead of this approach, could we maybe introduce a preprocessor #define that specifies how the parity of bit 51 relates to quiet / signalling, and use that in constructing the quiet NaN from a bit pattern?

---

Gah. Bit 51, not bit 52; edited the above text to correct.

[mdickinson]

Should probably use Py_INFINITY here, since we're changing this line anyway. (I don't want to clutter this PR by changing all instances of Py_HUGE_VAL to Py_INFINITY; that can happen in a separate cleanup PR.)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit fd39437 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[mdickinson]

FTR, my comment above is out of date; see the linked issue for discussion.

[mdickinson]

@seberg I've started a buildbot run. I'm happy with the shape of this PR if the buildbots agree.

We're missing a news entry; do you want to add one, or shall I? (I guess most of this could be classed as internal refactoring, which doesn't really need a news entry, but the MIPS fix probably deserves something.)

[seberg]

Happy either way, you probably got a more precise angle on what to write.

The Py_INFINITY definition may be the most interesting part for a news entry (but the MIPS fix doesn't hurt).

[mdickinson]

Updated to add a news entry and fix another couple of Py_HUGE_VAL instances. I'll start another buildbot run, and merge if the buildbots look happy. @seberg Thank you for the PR!

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit fabf7d3 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[mdickinson]

LGTM, subject to buildbot happiness.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit e8c36a2 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[mdickinson]

Okay, we're getting relevant buildbot failures. E.g., from here

======================================================================
FAIL: test_nan_signs (test.test_float.InfNanTest.test_nan_signs)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\pull_request.bolen-windows10\build\Lib\test\test_float.py", line 1045, in test_nan_signs
    self.assertEqual(copysign(1.0, float('nan')), 1.0)
AssertionError: -1.0 != 1.0
----------------------------------------------------------------------

Looks like we will need that fabs call around the NAN macro (which isn't really surprising - there's nothing in the C standard that says that NAN should have any particular sign bit).

The question is whether we want to put the fabs in just for the float("nan") and math.nan uses (and complex equivalents) and leave Py_NAN as the "system" NaN, or include the fabs call in the Py_NAN macro.

Dismissing review until we can get tests passing on all buildbots

[seberg]

Hmmmpf, I underestimated the complexity... Would be tempted to include it in the macro but don't have a real opinion or gut feeling on it (assuming any reasonable compiler optimizes it well).
The reason is that there are three places where it matters probably and 2 of them use -Py_NAN (all other places are probably errors anyway). OTOH, of course in typical use it shouldn't matter at all.

(NumPy has a similar use of -NPY_NAN although not sure it matters. Actually NumPy currently still hard-codes the NANf value and casts it. Somehow the cast from float to double apparently "fixes" the bit pattern on MIPS...)

[mdickinson]

I'm leaning towards leaving Py_NAN as a simple wrapper around NAN (or __builtin_nan) and only adding the fabs where we need it - i.e., for math.nan and float("nan").

My general philosophy on this is that most of the time we shouldn't care too much about signs of NaNs. float("nan") is an exception because it seems like too much of a violation of the POLS to have float("nan") give a negative NaN while float("-nan") gives a positive NaN. For math.nan, in the absence of backwards compatibility constraints I think I'd actually prefer math.nan just to return exactly what C's NAN does, but I also think that that would be an unacceptable backwards compatibility break, so we're stuck with forcing a positive NaN in this case.

[mdickinson]

FWIW, I believe that one time that people end up caring about signs of Python NaNs is when the NaN in question hits a C-level printf (in some extension module) and the printf implementation in question likes to put negative signs on negative NaNs. (The C standard doesn't seem to have anything to say about what printf should do here, and actual implementations seem to differ.)

[seberg]

OK, did the minimal thing and added fabs specifically where it seems possibly interesting (there is the Unpack2 for float16 unpacking that doesn't test yet, but used -Py_NAN explicitly).

I was assuming the __builtin_nan is just predating NAN in C99 requirement, but can undo.

For what its worth, NumPy creates NaNs all over with the same system dependent sign bit I suspect, and nobody ever complained.

EDIT/NOTE: Buildbot tests should be rerun probably.

[mdickinson]

Yep, agreed that getting rid of that use of __builtin_nan looks like the right thing to do - I would have done that in a followup PR if you hadn't done it here.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit a35ae19 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[mdickinson]

Thanks for the updates; that matches what I had in mind. Watching the buildbots (thanks @JelleZijlstra for triggering) ...

[mdickinson]

LGTM, though let's not merge until the buildbots finish. (I'm seeing two buildbot failures so far, but they're both clearly unconnected to this PR.)

[mdickinson]

Buildbots are happy, and so am I. Merging. Thank you, @seberg!

I'll try to find time shortly (though not today) to replace remaining occurrences of Py_HUGE_VAL with Py_INFINITY (at least, in those places where infinity is clearly what's intended, which I suspect is all of them).