radiocosmology / caput

Cluster Astronomical Python Utilities
GNU General Public License v2.0
12 stars 21 forks source link

Truncate float NaN behaviour is inconsistent across systems #280

Closed ljgray closed 1 month ago

ljgray commented 1 month ago

The following function calls

assert truncate.bit_truncate_float(32.121, np.nan) == 0
assert truncate.bit_truncate_double(32.121, np.nan) == 0

are true on linux but false on macos. Windows is untested.

With macos, calling bit_truncate_float/bit_truncate_double with err=np.nan evaluates the same as err=0.0 and returns the input argument - in this case, 32.121. This behaviour is different from err=-1 and err=np.inf, which both correctly evaluate to 0.0.

On linux (Ubuntu, at least), all three cases evaluate to 0.0.

Update: it appears that calling

np.asarray(np.nan).astype(int)

on macos evaluates to 0, wheras on linux it evaluates to -9223372036854775808, the smallest int64 value. It seems that numpy's solution is to throw a RuntimeWarning: https://github.com/numpy/numpy/issues/21166

@ketiltrout Do you have any thoughts on how we should handle this in truncate?

ketiltrout commented 1 month ago

Probably we should check if err is any sort of NAN (i.e. err != err), and just raise ValueError in that case, because what are we supposed to assume the caller meant?

If we don't want to do that, we should explicity set err to np.inf (i.e. act as if there's no limit) in this case.

Does this need to be handled in trunacte.hpp (i.e. are there other places using the C++ functions directly), or can we put it in the boilerplate in truncate.pyx (where' it's easier to raise exceptions)?

ketiltrout commented 1 month ago

(If in the .hpp, the C++-ish equivalent to raising an exception would probably be to just return NaN in this case).

ljgray commented 1 month ago

@ketiltrout Do you a preference for doing this check in the .pyx vs the .hpp file?

ketiltrout commented 1 month ago

I would do it in the .pyx. The Python-ier the better.