Deprecate nextpow2/prevpow2 #28304
Deprecate nextpow2/prevpow2 #28304
Conversation
|
just to be sure @nanosoldier |
|
Actually, that's stupid. There'll be a performance regression until the caller upgrade because deprecations are expensive. Is there a command to cancel nanosoldier? |
| @@ -297,7 +297,6 @@ export | |||
| muladd, | |||
| nextfloat, | |||
| nextpow, | |||
| nextpow2, | |||
|
I'm fine with these but we might also want to consider |
Yes, you can ping me and I'll ssh into the worker node and manually kill the process. 😛 (That only works well when only one job is running though, because Reasons™) |
base/intfuncs.jl
Outdated
| # Special case fast path for Integer/Unsiged a == 2. | ||
| # This is a very common case. Constant prop will make sure that a call site | ||
| # specified as `nextpow(2, x)` will get this special case inlined. | ||
| a == 2 && (isa(x, Integer) || isa(x, Unsigned)) && return _nextpow2(x) |
There was a problem hiding this comment.
Unsigned <: Integer, so isn't this equivalent to just checking isa(x, Integer)?
ararslan
added
maths
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
I'm fine with |
base/intfuncs.jl
Outdated
| # Special case fast path for x::Integer, a == 2. | ||
| # This is a very common case. Constant prop will make sure that a call site | ||
| # specified as `nextpow(2, x)` will get this special case inlined. | ||
| a == 2 && isa(x, Integer) && return _nextpow2(x) | ||
| a <= 1 && throw(DomainError(a, "`a` must be greater than 1.")) | ||
| x <= 0 && throw(DomainError(x, "`x` must be positive.")) |
There was a problem hiding this comment.
I think this error check needs to come before the check for a == 2. That's causing the test failures, since _nextpow2(x) won't throw a DomainError for x < 1.
There was a problem hiding this comment.
Which does present a problem though, because the old nextpow2 and prevpow2 functions did not throw an error for x == 0 and returned the next negative power of two for negative integers.
There was a problem hiding this comment.
That seems like a marginal and debatable behavior that we could break. Or if it’s really useful, we could generalize it. But I suspect it’s not. Negative powers of two are in fact not powers of two at all.
There was a problem hiding this comment.
I'll just move the <= 0 check to the top for now; seems like the conservative thing to do.
JeffBezanson
force-pushed
the
kf/nextpow2
branch
from
5d221e5
to
f5b4aeb
Compare
JeffBezanson
force-pushed
the
kf/nextpow2
branch
from
f5b4aeb
to
d3d8efc
Compare
Keno
force-pushed
the
kf/nextpow2
branch
2 times, most recently
from
eaef6ed
to
e9be0c5
Compare
Base 2 is an important special case here, but we can just handle that as a special case in the implementation of `nextpow` rather than having a whole separate user facing function.
Base 2 is an important special case here, but we can just handle that
as a special case in the implementation of
nextpowrather thanhaving a whole separate user facing function.
Note: Done against kf/squeeze, since otherwise NEWS.md conflicts unnecessarily. Merge that one before this and all will be fine.