ENH: Add the C99 remainder function, as `np.remainder_ieee` #9963

eric-wieser · 2017-11-05T09:27:22Z

Implementation stolen from cpython 3.7.

As suggested in a comment in #9702.

mhvk

This looks good; just seems there are a few statements left in from testing.

I wish there was a way to change the meaning of the name, especially since we already have np.mod which is just the same thing as the current np.remainder. But the only way I can think of doing that is to add an argument to np.remainder, which could warn upon the "wrong" setting; this seems non-trivial for a ufunc.

mhvk · 2017-11-05T16:27:18Z

numpy/core/src/npymath/npy_math_internal.h.src

+               will be computed exactly, with no rounding error
+               introduced.
+            */
+            assert(m == c);


Doesn't hurt and might turn up potential problems. This comes from the Python implementation.

mhvk · 2017-11-05T16:28:22Z

numpy/core/src/npymath/npy_math_internal.h.src

+    if (npy_isfinite(x)) {
+        return NPY_NAN;
+    }
+    assert(npy_isfinite(y));


Left-over from testing?

eric-wieser · 2017-11-05T18:16:06Z

asserts are there in cpython too.

eric-wieser · 2017-11-05T19:18:32Z

I muddled isfinite and isinfinite there - fixed now. Is there a reason we have no npy_isinfinite? Or why we don't just use the python macros?

EDIT: I am dumb, npy_isinf is a thing

rgommers · 2017-11-05T19:24:17Z

I'm not a big fan of the name, nor of many almost identical but separate functions. Have you considered any alternatives, like np.remainder(..., method='ieee')?

charris · 2017-11-05T19:28:32Z

@rgommers Maybe pyremainder? Adding a method keyword to a ufunc would be tricky. I'm not a big fan of the name either ...

charris · 2017-11-05T19:31:44Z

I suppose the function could be put somewhere else with a method keyword, but that is a more intrusive change.

eric-wieser · 2017-11-05T19:49:59Z

An argument for the current name - it matches java.lang.Math.IEEEremainder

mhvk · 2017-11-05T19:50:24Z

@charris - I wondered about turning remainder into a front-end function as well, which either calls mod or the new remainder ufunc, but then we'd still need the ufunc anyway, so it seems that it is an independent discussion.

rgommers · 2017-11-05T19:52:55Z

I'd also like to see the argument for adding this in the first place. The differences with np.remainder seem small. Current examples:

In [1]: np.remainder([4, 7], [2, 3])
Out[1]: array([0, 1])

In [2]: np.remainder(np.arange(9), 4)
Out[2]: array([0, 1, 2, 3, 0, 1, 2, 3, 0])

Two differences; this implementation gives back floats rather than ints for int input, and the second one changes 3 to -1 in output. Unless I'm missing something major, that's a fairly marginal change.

eric-wieser · 2017-11-05T19:56:12Z

this implementation gives back floats rather than ints for int input

This is an omission and not a feature.

the second one changes 3 to -1 in output
Unless I'm missing something major, that's a fairly marginal change.

This is as marginal a change as round(1.5) == 2 vs floor(1.5) == 1. For each type of conversion to integer, there is a corresponding type of remainder.

So yes, there would be a good argument here for np.remainder(..., rounding={'even', 'down', 'up', 'towards-zero'}), but in practice we'd want each of those to have their own ufunc for handling by __array_ufunc__

rgommers · 2017-11-05T19:56:30Z

but then we'd still need the ufunc anyway, so it seems that it is an independent discussion.

Doesn't mean that it would need to be public though.

n argument for the current name - it matches java.lang.Math.IEEEremainder

Hmm... we want to choose nice functional names, not match other languages. And of those others, Java is very low on the recognition scale so not very relevant.

eric-wieser · 2017-11-05T20:01:43Z

Hmm... we want to choose nice functional names, not match other languages. And of those others, Java is very low on the recognition scale so not very relevant.

However, we also don't want to mismatch other languages, which is the problem remainder currently has. But yes, this is not a strong argument for ieee_remainder.

Doesn't mean that it would need to be public though.

It needs to be visible to __array_ufunc__, which is almost public

I suppose we could move the functions to:

np.remainder(..., rounding='downward')
np.remainder.downward # old remainder
np.remainder.to_nearest   #this
# later: to_zero, upward

rgommers · 2017-11-05T20:03:20Z

This is as marginal a change as round(1.5) == 2 vs floor(1.5) == 1.

Not quite I think. round and floor are much more common operations. If the majority opinion is to add this function I won't block it, but my opinion is that this is too marginal to add to the main numpy namespace.

eric-wieser · 2017-11-05T20:06:41Z

my opinion is that this is too marginal to add to the main numpy namespace.

Does that opinion also stand against np.remainder(..., rounding='even') , or simply against the function having it's own name.

I suppose this is similar to #9696, in that python adding things to math is not a sufficient justification to us adding things to np

rgommers · 2017-11-05T20:08:45Z

Does that opinion also stand against np.remainder(..., rounding='even') , or simply against the function having it's own name.

Adding as an extra method inside np.remainder would be much better, I'd be in favor of that. Reason: we get the same extra functionality, but not the downsides of namespace size growth.

eric-wieser · 2017-11-05T21:44:45Z

Unfortunately adding the extra method is a lot of work without breaking compatibility of isinstance(np.remainder, np.ufunc) and all the properties that entails.

Would you be happier with np.remainder_ieee, which is more discoverable by autocomplete?

rgommers · 2017-11-06T09:59:56Z

Would you be happier with np.remainder_ieee, which is more discoverable by autocomplete?

Better, but doesn't make much of a difference to my opinion. If we're bikeshedding names, I don't think something like ieee belongs in a function name; names should be functional not named after entities.

charris · 2017-11-06T18:12:06Z

@rgommers I don't see the difficulty here. I think we need the function for completeness, both for Python compatibility and because is has greater precision on the near negative side of zero, which is where it is useful due to the distribution of floating point numbers. The main question is how to name it so that it does not collide with our long time remainder function and so that it carries some meaning for the user. I think remainder_ieee is about as good as we are going to get in that direction. If we later decide to add a pseudo ufunc with a method, we will still need an underlying ufunc, and I see no reason to hide it.

EDIT: It would have been nice if the Python folks had been more cognizant of the NumPy tradition here, but it is too late to change that.

charris · 2017-11-06T18:29:07Z

Hmm, I suppose __array_ufunc__ would still get called for the underlying computations, so perhaps a higher level implementation would not be so bad.

EDIT: Although there would still be a name collision, which might get worse if ufuncs and multiarray get combined somewhere down the line.

mhvk · 2017-11-06T20:38:07Z

@charris - I'd personally like turning remainder in a function that has a keyword that decides which underlying ufunc to call, and, as you noted, those ufuncs would be properly caught with __array_ufunc__. The only real annoyance is that, as @eric-wieser mentioned, this would break isinstance(remainder, ufunc).

In any case, I think we should have the specific ufunc and might then as well expose it. remainder_ieee is a bit better. I guess remainder_from_rounding or remainder_from_nearest or remainder_to_nearest are all rather too long?

charris · 2017-11-06T21:46:04Z

Another option is to let folks roll their own, a - around(a). However, there may be subtleties that such a simple implementation misses, for example, it is supposed to be independent of rounding mode.

mhvk · 2017-11-06T21:53:02Z

I do think we should have this as a ufunc exactly because of those subtleties. For this PR, I think the main question really is about the name!

eric-wieser · 2017-11-07T03:53:15Z

It sounds like remainder_ieee is preferred over ieee_remainder, so I'll update this. I think starting with remainder is probably more valuable than matching the java name.

Implementation stolen from cpython 3.7

rgommers · 2017-11-07T10:49:09Z

@rgommers I don't see the difficulty here.

It's simple math to me:

people that have a use for this function: close to zero
costs: more cognitive load for users, maintenance overhead for us

I think we need the function for completeness, both for Python compatibility and because is has greater precision on the near negative side of zero, which is where it is useful due to the distribution of floating point numbers.

Completeness is in the eye of the beholder. This is nothing like rounding, where we clearly do need the two behaviors for rounding .5. remainder is not a heavily used function, and >99% of our users will not even know of this definition of it (I certainly didn't either). Of those that do, there's again only a small fraction that would need the last ulps of precision that you cannot get from the simple one-liner.

charris · 2017-11-09T18:12:25Z

numpy/core/tests/test_umath.py

@@ -1268,6 +1268,25 @@ def test_heaviside(self):
        assert_equal(h, expected1.astype(np.float32))


+class TestIeeeRemainder(object):
+    def test_remainder_ieee(self):
+        # Testing is basic because implementation is copied from cpython, and


We need to test the different float types, no?

charris · 2017-11-09T18:13:09Z

numpy/core/tests/test_umath.py

@@ -1268,6 +1268,25 @@ def test_heaviside(self):
        assert_equal(h, expected1.astype(np.float32))


+class TestIeeeRemainder(object):


Needs rename?

charris · 2017-11-09T18:17:00Z

I think we should support most C99 functions because if someone wants to generalize a C, or even a Python 3.7+ program, to use ndarrays, then they may need the function. I do think this needs more tests.

eric-wieser · 2017-11-14T07:54:13Z

I do think this needs more tests.

Ideally we'd copy the python3 tests there, but to do so we need to implement #9969.

mattip · 2018-06-13T23:36:25Z

@mhvk is there a way to remove your "review approval" from this PR? The idea seems still controversial

@mattip

On request of @mattip, since the idea itself still seemed controversial

mhvk · 2018-06-14T00:26:03Z

@mattip - I dismissed my own review.

eric-wieser · 2019-05-11T17:50:47Z

Needs #13375 to port the tests too

eric-wieser · 2019-12-06T12:01:50Z

Now that C99 is a requirement, I'm inclined to remove the manual implementation, which also removes the need for detailed testing.

eric-wieser added 01 - Enhancement component: numpy._core labels Nov 5, 2017

mhvk previously approved these changes Nov 5, 2017

View reviewed changes

eric-wieser force-pushed the remainder branch from f35f198 to 3a5a475 Compare November 5, 2017 18:53

ENH: Add the C99 remainder function, as np.ieee_remainder

3915399

Implementation stolen from cpython 3.7

eric-wieser force-pushed the remainder branch from 3a5a475 to 3915399 Compare November 7, 2017 03:57

charris changed the title ~~ENH: Add the C99 remainder function, as np.ieee_remainder~~ ENH: Add the C99 remainder function, as np.remainder_ieee Nov 9, 2017

charris reviewed Nov 9, 2017

View reviewed changes

eric-wieser mentioned this pull request Mar 16, 2018

ENH: Implement np.floating.as_integer_ratio #10741

Merged

eric-wieser added the 59 - Needs tests label Jun 14, 2018

charris added the 25 - WIP label May 11, 2019

mattip mentioned this pull request Dec 5, 2019

Decide on the UFunc Inner loop signature and setup/teardown API seberg/numpy#12

Open

eric-wieser mentioned this pull request Dec 6, 2019

Add umath functions to wrap radian angles #2868

Closed

eric-wieser mentioned this pull request Aug 20, 2020

ENH: Phase unwrapping generalized to arbitrary interval size #16987

Merged

4 tasks

Base automatically changed from master to main March 4, 2021 02:04

charris added the 52 - Inactive Pending author response label Apr 6, 2022

charris closed this Apr 6, 2022

		@@ -1268,6 +1268,25 @@ def test_heaviside(self):
		assert_equal(h, expected1.astype(np.float32))


		class TestIeeeRemainder(object):

ENH: Add the C99 remainder function, as np.remainder_ieee #9963

ENH: Add the C99 remainder function, as np.remainder_ieee #9963

Conversation

eric-wieser commented Nov 5, 2017

mhvk left a comment

Choose a reason for hiding this comment

mhvk Nov 5, 2017

Choose a reason for hiding this comment

charris Nov 9, 2017

Choose a reason for hiding this comment

mhvk Nov 5, 2017

Choose a reason for hiding this comment

eric-wieser commented Nov 5, 2017

eric-wieser commented Nov 5, 2017 • edited Loading

rgommers commented Nov 5, 2017

charris commented Nov 5, 2017

charris commented Nov 5, 2017

eric-wieser commented Nov 5, 2017 • edited Loading

mhvk commented Nov 5, 2017

rgommers commented Nov 5, 2017

eric-wieser commented Nov 5, 2017 • edited Loading

rgommers commented Nov 5, 2017

eric-wieser commented Nov 5, 2017 • edited Loading

rgommers commented Nov 5, 2017

eric-wieser commented Nov 5, 2017

rgommers commented Nov 5, 2017

eric-wieser commented Nov 5, 2017

rgommers commented Nov 6, 2017

charris commented Nov 6, 2017 • edited Loading

charris commented Nov 6, 2017 • edited Loading

mhvk commented Nov 6, 2017

charris commented Nov 6, 2017

mhvk commented Nov 6, 2017

eric-wieser commented Nov 7, 2017 • edited Loading

rgommers commented Nov 7, 2017

charris Nov 9, 2017

Choose a reason for hiding this comment

charris Nov 9, 2017

Choose a reason for hiding this comment

charris commented Nov 9, 2017

eric-wieser commented Nov 14, 2017

mattip commented Jun 13, 2018

mhvk commented Jun 14, 2018

eric-wieser commented May 11, 2019

eric-wieser commented Dec 6, 2019

ENH: Add the C99 remainder function, as `np.remainder_ieee` #9963

ENH: Add the C99 remainder function, as `np.remainder_ieee` #9963

eric-wieser commented Nov 5, 2017 •

edited

Loading

eric-wieser commented Nov 5, 2017 •

edited

Loading

eric-wieser commented Nov 5, 2017 •

edited

Loading

eric-wieser commented Nov 5, 2017 •

edited

Loading

charris commented Nov 6, 2017 •

edited

Loading

charris commented Nov 6, 2017 •

edited

Loading

eric-wieser commented Nov 7, 2017 •

edited

Loading