Skip to content
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

Math module - rename isnan #19019

Closed
lydia-duncan opened this issue Jan 14, 2022 · 4 comments · Fixed by #23070
Closed

Math module - rename isnan #19019

lydia-duncan opened this issue Jan 14, 2022 · 4 comments · Fixed by #23070

Comments

@lydia-duncan
Copy link
Member

This function should be camelCased. Whether that means isNan or isNAN is still up for discussion via #6698. Whatever decision is reached should likely be consistent with #19016

@damianmoz
Copy link

If I could be so bold, there is no such thing as an Nan. There is a Not-a-Number which has already been abbreviated to NaN back in the 1980s. There are two types. One is a quiet NaN and the other is a signalling NaN, abbreviated to qNaN and sNaN respectively. The difference is a single bit in the bit-wise representation. Whether hardware supports signalling NaN s is another thing.

The name used in the C standard for such a quiet NaN is NAN. Is this the best name? Well, it is in that standard. I cannot fight it.

So, if you are going to camel case something, and for a 5 letter routine name, I question that but I am going to loose that argument even though the name has been around for 35+ years, it should be isNaN(). You cannot camel-case an already camel-cased name like NaN.

Note that with your newly camel-cased name

assert(isNaN(x) == (x != x));

This routine was introduced because some compilers' optimizers were flawed and were too broken to realise that in an IEEE 754 world that gioven a real(w) identifier, the expressions

x == x
x != x

cannot be determined at compile-time and need to be evaluated at run-time. So the isnan() routine, preferably expanded as an inline macro, was introduced way back in the 1980s to get around that broken-ness.

Your manual should be suggesting that Chapel users avoid the use of isNaN() and instead use the boolean expression form. It is more precise and is/was-always the way it is supposed to be written. Why? Because those users have the benefit of a great team of compiler writers who create a masterful optimizing compiler than does not barf at the sight of those boolean expressions,

I personally do not use the routine, I use the expression but then I ensure that I am not using krap compilers. The last time I checked, the chpl compiler did the right thing with the boolean expression form.

@mppf
Copy link
Member

mppf commented Jan 18, 2022

Regarding isNan vs isNaN vs isNAN, if we went with isNan it would just be because we will camel case acronyms that way (e.g. renderHtmlDom). See #6698 (comment) .

@lydia-duncan
Copy link
Member Author

Prep for an upcoming ad-hoc subteam discussion

1. What is the API being presented?

  /* Returns `true` if the argument `x` does not represent a valid number;
     `false` otherwise. */
  inline proc isnan(x: real(64)): bool

  /* Returns `true` if the argument `x` does not represent a valid number;
     `false` otherwise. */
  inline proc isnan(x: real(32)): bool

How is it intended to be used?

Damian points out that it was originally added because some compilers’ optimizers couldn’t recognize that x == x and x != x couldn’t be determined at compile time and needed to be evaluated at runtime. The last time he checked, Chapel did the right thing with the boolean expression form, so isnan is not really necessary.

How is it being used in Arkouda and CHAMPS?

In Arkouda:

In CHAMPS:

  • There is 1 use in geo/geo.chpl
  • There is 1 use in drop/drop.chpl
  • There is 1 use in common/src/baseClasses.chpl
  • There is 1 use in flow/flow.chpl
  • There is 1 use in thermo.chpl

2. What's the history of the feature, if it already exists?


These functions predated the proc and inline keywords. Both were originally added in 2007. The 64 bit version being added alongside support for complex numbers, which it was used to support. The 32 bit version support was added shortly after, alongside 32 bit versions of the other math functions.

They were originally extern functions, and when extern functions started generating prototypes, needed pragma “no prototype” to avoid errors due to the C support using macros.

Their implementation was adjusted in 2012 to avoid using pragma “no prototype” and support them naturally in Chapel, even though we are relying on macros in C under the covers. This transformed them into inline functions that wrap runtime calls.

Documentation for these functions was added in 2015.

There was an attempt to unify the overloads of each into a single function, but this was thwarted by promotion.

Were moved to AutoMath.chpl when the split happened so that they could continue to be included in user programs by default.

3. What's the precedent in other languages, if they support it?

a. Python

Python provides isnan (https://docs.python.org/3/library/math.html#math.isnan). NumPy uses the same capitalization scheme (https://numpy.org/doc/stable/reference/generated/numpy.isnan.html#numpy.isnan)

b. C/C++

C/C++ provides isnan (https://numpy.org/doc/stable/reference/generated/numpy.isnan.html#numpy.isnan) and are, I believe, where we got the name from

c. Rust

Rust provides is_nan (https://doc.rust-lang.org/std/primitive.f64.html#method.is_nan)

d. Swift

Like isInfinite, Swift provides isNaN as a property on doubles (https://developer.apple.com/documentation/swift/double/isnan)

e. Julia

Julia provides isnan (https://docs.julialang.org/en/v1/base/numbers/#Base.isnan)

f. Go

Go provdes isNaN (https://pkg.go.dev/math#IsNaN)

4. Are there known Github issues with the feature?


5. Are there features it is related to? What impact would changing or adding this feature have on those other features?

6. How do you propose to solve the problem?


A. Rename to isNan

  • Pros:
    • Most literal interpretation of our standard module style guide
  • Cons:
    • Not in keeping with any other language
    • Kinda ugly

B. Rename to isNaN

  • Pros:
    • Nicer looking
    • In keeping with Swift and Go
  • Cons:
    • Not in keeping with Python, C/C++, Rust, or Julia

C. Deprecate it entirely?

  • Pros:
    • One less thing to maintain going forward
    • Arguably already supported by != and == comparisons, as Damian has found
  • Cons:
    • All other languages provide this function, even the more modern ones

I think we should follow B. We should also add the documentation note Damian suggested

@lydia-duncan
Copy link
Member Author

In our discussion today, we decided to rename this to isNan. Though isNaN had support, its support was not a strong as isNan.

This was decided by looking at the combination of this name and the naming of NAN, where the decision for that has already been posted on #19016

Discussion points:

  • An argument was made that there's no other concept nan would refer to.
  • We did consider deprecating it, but there was not strong support

We are willing to meet again if new counterpoints are presented.

lydia-duncan added a commit that referenced this issue Aug 28, 2023
[reviewed by @ShreyasKhandekar]

Renames:
- `INFINITY` to `inf`
- `NAN` to `nan`
- `isfinite` to `isFinite`
- `isinf` to `isInf`
- `isnan` to `isNan`

These functions were not following the standard module style
guide rules and so needed to be adjusted.

Resolves #19016 
Resolves #19017 
Resolves #19019

Updated documentation links to refer to the new symbol rather
than the old one.

Added tests to lock in the deprecation messages

Updated tests that were using the deprecated versions to use
the new versions instead.

Passed a full paratest with futures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants