-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
ENH: Add gcd and lcm ufuncs #8774
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
Conversation
numpy/core/src/umath/loops.h.src
Outdated
U@TYPE@_absolute(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED(func)); | ||
|
||
NPY_NO_EXPORT void | ||
@TYPE@_absolute(char **args, npy_intp *dimensions, npy_intp *steps, void *NPY_UNUSED(func)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit's diff is just combining U@TYPE@_absolute
and @TYPE@_absolute
into @S@@TYPE@_absolute
, and the same for _sign
, _divide
, and _remainder
. This is already the pattern we use for other functions in this file
Not the most efficient of algorithms - CPython 3.5 uses Lehmer's algorithm in Also, the builtin |
160c2a5
to
264c1c7
Compare
Ok, tests are added
|
f881013
to
24774f6
Compare
All green, documented, and release note'd |
☔ The latest upstream changes (presumably #8795) made this pull request unmergeable. Please resolve the merge conflicts. |
/**end repeat**/ | ||
|
||
/**begin repeat | ||
* #TYPE = UBYTE, USHORT, UINT, ULONG, ULONGLONG# | ||
* #type = npy_ubyte, npy_ushort, npy_uint, npy_ulong, npy_ulonglong# | ||
* #c = u,u,u,ul,ull# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard codes for these are B
, H
, I
, L
and Q
. Shouldn't you use those instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. Although I didn't see much point implementing B
, H
, and I
separately, because arithmetic always promotes to int
in C anyway, doesn't it?
Can you link me to a file that uses these standard codes? Oh, these are the struct.pack
codes, right? I was trying to write this as though it were a free C function, like labs
and llabs
are to abs
. I guess in hindsight they use a prefix though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better example of what I was imitating might be strto{l,ll,ul,ull}
from the C standard library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...these are the struct.pack codes, right?
They are also documented in numpy; see https://docs.scipy.org/doc/numpy/reference/arrays.scalars.html#built-in-scalar-types and numpy.typecodes['Integer']
, numpy.typecodes['UnsignedInteger']
, etc.
☔ The latest upstream changes (presumably #8847) made this pull request unmergeable. Please resolve the merge conflicts. |
d282345
to
40b4bc8
Compare
Would be quite nice to have this! To me, it looks OK, but maybe e.g. @juliantaylor can have a look? |
adca9d6
to
36d6cc7
Compare
TD('O', f='npy_ObjectGCD'), | ||
), | ||
'lcm' : | ||
Ufunc(2, 1, None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got this wrong until now. The identity of gcd is 0
since by definition, gcd(a, 0) == a
.
The identity for lcm
is some kind of epsilon. For the integers, that epsilon is simply 1
(lcm(a, 1) = a
). However, for the rationals, (ie decimal.Decimal
), this is incorrect (lcm(0.2, 1) != 0.2
), nor is such an epsilon really representable anyway.
☔ The latest upstream changes (presumably #9063) made this pull request unmergeable. Please resolve the merge conflicts. |
36d6cc7
to
7278d17
Compare
Rebased for 1.14 |
7278d17
to
d1513be
Compare
I looked again at this, and it all looks fine, except there was the question about which character to use. Also, I'd explicitly check that calling the functions with a float gets you an error, and maybe that it works for a python super-long integer. |
Now works for builtin `long`, `Decimal`, and `Fraction` types
@mhvk: Tests and release notes fixed. I don't think there's a strong argument to be made about the suffices - since they're a private implementation detail, I'd prefer to leave them |
OK, that looks great. Merging! |
Thanks! Glad I won't have to rebase for 1.16 too! |
Fixes #8772, implementing Least Common Multiple and Greatest Common Divisor as
ufunc
sSince these are now both in the C++17 stdlib, it seems pretty reasonable that they should be part of numpy as well. Specification matches the C++17 one, where the result is always positive.
Defined for:
int
sobject
long
s defer tomath.gcd
(if available)Decimal
,Fraction
, ...) defers tofraction.gcd
np.core.internal._gcd
, but enforcing consistency withmath.gcd
(returning a positive value)Not defined for:
bool
- doesn't seem useful, since it just decays tological_or
np.inexact
- not well definedFirst commit is some minor cleanup to make it easier to see where to add new integer ufuncs. Githubs rendering of this diff is not very clear