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

ENH, MAINT: Fix behavior of int and bool in np.spacing #13931

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kianasun
Copy link
Contributor

@kianasun kianasun commented Jul 7, 2019

When the dtype of the input is int or bool, the output of np.spacing function was puzzling.

@eric-wieser
Copy link
Member

the output of np.spacing function was puzzling.

For ease of reviewing, can you show what the output was?

@kianasun
Copy link
Contributor Author

kianasun commented Jul 7, 2019

the output of np.spacing function was puzzling.

For ease of reviewing, can you show what the output was?

Sure. Below is what the output was

>>> np.spacing(np.ones(3, dtype=np.int))
array([2.22044605e-16, 2.22044605e-16, 2.22044605e-16])
>>> np.spacing(np.ones(3, dtype=np.bool))
array([0.000977, 0.000977, 0.000977], dtype=float16)
>>> np.spacing(np.ones(3, dtype=np.float64))
array([2.22044605e-16, 2.22044605e-16, 2.22044605e-16])

@eric-wieser
Copy link
Member

Sounds like our option is to either accept this patch, or update the docs to say that spacing is for floating point types.

How does the behavior of nextafter compare?

@kianasun
Copy link
Contributor Author

kianasun commented Jul 8, 2019

>>> eps = np.finfo(np.float).eps
>>> np.nextafter(np.int(1), np.int(3)) == eps + 1
True
>>> np.nextafter(np.float(1), np.float(3)) == eps + 1
True
>>> np.nextafter(False, True)
6e-08

For nextafter, it seems that at least int is same asfloat. Yes, we can either change the behaviors, or update the docs and raise some errors for int and bool.

@eric-wieser
Copy link
Member

eric-wieser commented Jul 8, 2019

You're seeing the same behavior for both functions.

  • f(some_int64)f(some_int64.astype(np.float64))
  • f(some_bool)f(some_bool.astype(np.float16))

This is perhaps not the best design, but it is consistent with how casting works in numpy (cc @seberg), eg for functions like np.sin.

It's worth noting that C++ takes a different approach here, producing an error if a cast is ambiguous. If we could redo casting without breaking everyone downstream, following the overload design of C++ might be the way to go.


An aside - never use np.float or np.int - they're just confusing names for the builtin types, as summarized in #6103.

@charris charris changed the title MAINT: Change behaviors of int and bool in spacing MAINT: Fix behavior of int and bool in np.spacing Jul 8, 2019
@charris charris changed the title MAINT: Fix behavior of int and bool in np.spacing ENH, MAINT: Fix behavior of int and bool in np.spacing Jul 8, 2019
@charris
Copy link
Member

charris commented Jul 8, 2019

Needs release note.

@seberg
Copy link
Member

seberg commented Jul 8, 2019

@eric-wieser yeah, I agree. The default TypeResolver uses the first loop which can cast all inputs safely. That kind of works, but really is more trouble than worth it it seems to me.

For when we redo this, I think most of these fallbacks should be deprecated, it is OK if the ufunc has to resolve specifically integer inputs -> use float64 (I think it actually might be a point that a true hierarchy could be convenient). We could go with an error, for np.sin, etc. the nicest thing would likely be to go for float64? That might be easiest with a "hard" behaviour change maybe.

Seems a bit orthogonal to this issue. For this specific thing, we have to decide if we want to rather go with a Deprecation rather than defining a spacing. Does anyone know a real use-case for defining spacing on int (where the intention is not to cast them to floats)? Without a use case, I am in favor of going the deprecation route here I think.

@kianasun
Copy link
Contributor Author

kianasun commented Jul 9, 2019

@eric-wieser @seberg I am not sure if there is a real use-case for this specific function. In that case, will it be better to simply change the document? If we change the document, do we need to raise errors/warnings for int, bool? If we need to raise errors, we are still changing the behaviors (might need to give int a higher priority for TypeResolver to deal with, before safely casting them to float). And at least for bool, casting them to float seems incorrect I think.

@seberg
Copy link
Member

seberg commented Jul 9, 2019

@kianasun, my comments about TypeResolving were more philosophical for a future change API change. We can deprecate using this functions on integers (it involves creating a new TypeResolver right now).

@charris
Copy link
Member

charris commented Jul 14, 2019

close/reopen to retrigger shippable.

@charris charris closed this Jul 14, 2019
@charris charris reopened this Jul 14, 2019
Base automatically changed from master to main March 4, 2021 02:04
@InessaPawson InessaPawson added the 52 - Inactive Pending author response label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending author's response
Development

Successfully merging this pull request may close these issues.

None yet

5 participants