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

WIP, ENH: Scalarmath cleanup #18682

Closed
wants to merge 6 commits into from
Closed

Conversation

ganesh-k13
Copy link
Member

@ganesh-k13 ganesh-k13 commented Mar 27, 2021

Cleanup of Scalar math

This PR is an updated version of a WIP branch. I have tried to get it to a partially working state.

A couple of bugs and few other enhancements I tried, all derive from this branch as the base. So I started work on fixing it.

This requires massive amounts of testing before merge. But I am creating this PR to have a centralized location to go over queries I have in the Scalar math code base, which I have many :).

CC: @seberg

@seberg
Copy link
Member

seberg commented Mar 29, 2021

Cool! Just a note, there might be a small conflict now with gh-18691. But it probably is useful, since the tests added there are probably a great start on new tests for this as well.

@aarchiba
Copy link
Contributor

aarchiba commented Apr 3, 2021

As I did in #18691 you may find hypothesis a useful tool for checking a wide range of operators against a wide range of types. Perhaps, for example, checking that floating-point scalar arithmetic agrees with python floats to within a few epsilon (using the appropriate epsilon), or ensuring that mixed-type commutative operators commute exactly, in terms of output type and value. You could also write tests that check that array math agrees with math on the correponding scalars. hypothesis is also good at finding awkward values that trigger strange behaviour. We found a subtle arithmetic mistake in Astropy's Time object (times are represented as pairs of double-precision numbers) arithmetic by letting hypothesis explore the space..

@ganesh-k13
Copy link
Member Author

Thank you Anne! Hypothesis looks like a great tool. I was going over your PR and found out about hypothesis. I will try to add few cases with this framework that will be useful for this PR .

This function is used to be called `_typenum_fromtypeobj` but
since it is used in more places (including one new added now),
giving it a long descriptive name seemed reasonable.

Ideally, I would like to phase out the `user` option and expose
(or use) the new functionality in the array-coercion to handle
such cases making this only useful for super-fast paths of
our internal scalars.
This simplification and code change does a few things:

1. We remove all binop deferals, by simply only handling the numeric
   numpy scalars and python floats+integers specifically.
   All other object will cause deferal.
   This specifically defers subclassess even of NumPy scalars
   (unless it is `class+subclass` where Python will reverse the
   lookup.
2. We always defer binops if the type number of the current type
   is smaller.  This allows `float32 + float64` to retry with
   `float64 + float32`, which is much faster.
3. It simplifies the double special cases and expand it to
   include longdouble.

The previous casting code was faster, so cases such as:

    float64 + float32

were somewhat faster.  However, previously `float32 + float64`
would end up using the fallback path and thus be *much* slower.

Identically, `scalar + array0d` had previously a fast-path which
is now removed.  While, thus much slower, it means that the
performance is now symmetric wtih `array0d + scalar`.

Generally, it could make sense to make a dumbed down ufunc machinery
for 0D scalars, to strike a middle path between the current super-slow
array fallback and the somewhat quick scalar paths.

However, scalars do have the advantage that they can refuse to handle
anything that is not known much easier, i.e. they do not need to find
the common dtype. If the other scalar is not known, they can just defer
(or rather fall back to the slow paths).
@seberg
Copy link
Member

seberg commented May 6, 2022

Closing, since it is superceded by gh-21188 (not sure if there might be some additional ideas here, though).

@seberg seberg closed this May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants