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

ispow2(x) for non-Integer x #37635

Merged
merged 3 commits into from
Sep 21, 2020
Merged

ispow2(x) for non-Integer x #37635

merged 3 commits into from
Sep 21, 2020

Conversation

stevengj
Copy link
Member

Seemed like there is no reason to limit ispow2(x) to x::Integer.

@stevengj stevengj added the maths Mathematical functions label Sep 17, 2020
@stevengj
Copy link
Member Author

CI errors seem unrelated:

command timed out: 7320 seconds without output running [b'bin/julia', b'-e', b'include(joinpath(Sys.BINDIR, Base.DATAROOTDIR, "julia", "test", "choosetests.jl")); Base.runtests(append!(choosetests()[1], ["LibGit2/online", "download"]); ncores=min(Sys.CPU_THREADS, 8, 5))'], attempting to kill

on tester_macos64 and

command timed out: 3600 seconds without output running ['sh', '-c', 'make VERBOSE=1 TAGGED_RELEASE_BANNER="Official https://julialang.org/ release" SRCCACHE=/tmp/srccache USECCACHE=1 JULIA_CPU_TARGET="generic;cortex-a57;thunderx2t99;armv8.2-a,crypto,fullfp16,lse,rdm" MARCH=armv8-a  LLVM_ASSERTIONS=1 FORCE_ASSERTIONS=1 USE_BINARYBUILDER=1  binary-dist'], attempting to kill

on package_linuxaarch64.

"""
ispow2(x::Number) = isreal(x) && ispow2(real(x))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be restricted to Complex, since there isn't a fallback covering all Real values.

Copy link
Member Author

@stevengj stevengj Sep 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there should be a way to throw a MethodError for common patterns like this, rather than StackOverflowError. I've bumped into similar issues many times, e.g. with f(x) = f(float(x)) patterns.

Could we implement something like

@norecurse ispow2(x::Number) = isreal(x) && ispow2(real(x))

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For example, this Number method would work perfectly well for quaternion types. It seems a shame to lose genericity due to dispatch limitations.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

Copy link
Contributor

@JeffreySarnoff JeffreySarnoff Sep 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With @stevengj's approach ^, the work of making types consistent inside and collaboratively correct outside is a touch simpler.

@norecurse fn(x::Real) = fn(float(x))

dispatch resolves giving attentive safety

@JeffBezanson JeffBezanson merged commit 8659cfe into master Sep 21, 2020
@JeffBezanson JeffBezanson deleted the sgj/more-ispow2 branch September 21, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants