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

Fix sincos for reals that are not AbstractFloats #25292

Merged
merged 16 commits into from
Jan 24, 2018

Conversation

dpsanders
Copy link
Contributor

@dpsanders dpsanders commented Dec 28, 2017

Fixes #22095.

This PR adds a generic fallback for sincos.
This allows exp(a + im*b) to work correctly when the type of a and b is a non-standard subtype of Real (e.g. ArbFloat or Interval).

There is a test provided with the simplest such non-standard type.

[Note that this -- EDIT: that is, exp(a + im*b) -- worked in Julia 0.6, but does not work in current master, so this is in fact a bug fix for a regression and not a new feature.]

@@ -197,7 +197,9 @@ function sincos(x::T) where T<:Union{Float32, Float64}
return -co, si
end
end
sincos(x::Real) = sincos(float(x))
sincos(x::T) where {T <: Union{Integer, Rational}} = sincos(float(x))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about irrational?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's covered by the old definition that's kept below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but irrationals don't use the faster version, right? (I'm on my phone, I can't check myself right now)

Copy link
Contributor

@pkofod pkofod Dec 28, 2017

Choose a reason for hiding this comment

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

Oh, my bad, of course you're right, it should be {Integer, Rational, Irrational}. The current changes makes sincos(pi) slower than before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point, thanks, I'll change it.

I don't understand how to interpret the test errors -- can anybody help me?

@fredrikekre
Copy link
Member

LoadError("sysimg.jl", 320, LoadError("math.jl", 988, LoadError("special/trig.jl", 201, UndefVarError(:Irrational))))

@pkofod
Copy link
Contributor

pkofod commented Dec 28, 2017

Is irrational not defined at this point?

@dpsanders
Copy link
Contributor Author

Apparently not. Also, in the future we may want to define sin(pi) etc explicitly. So I think it's better to leave the generic definition to cover irrationals.

@pkofod
Copy link
Contributor

pkofod commented Dec 28, 2017

Practically speaking this should be of no real importance, but do you then propose that sincos(someotherrational) should be slow? Or do you want to define sin for all irrationals?

@dpsanders
Copy link
Contributor Author

I have now implemented @yuyichao's approach from #22095 (comment)

and left a generic fallback.
I think this is the best solution for now.

@dpsanders
Copy link
Contributor Author

The current failures seem unrelated (being in the show tests, not the math tests).

@fredrikekre
Copy link
Member

It is related

Error in testset show:
Test Failed at /tmp/julia/share/julia/test/show.jl:580
  Expression: contains(Base.url(first(methods(sin))), "https://github.com/JuliaLang/julia/tree/$((Base.GIT_VERSION_INFO).commit)/base/missing.jl#L")

@JeffBezanson
Copy link
Member

That is a very unfortunately specific test. It should be revised or removed so this can be merged.

@vtjnash
Copy link
Member

vtjnash commented Jan 17, 2018

Change first(methods(sin)) ==> which(sin, (Float64,)), so it's a bit less arbitrary than first (new file is "special/trig.jl")?

@dpsanders
Copy link
Contributor Author

I have now reimplemented the logic using multiple dispatch instead.
I fixed the overly-specific test following Jameson's suggestion.

Not sure what the test failures are about now. They seem unrelated?

@dpsanders
Copy link
Contributor Author

Made a big rebase/merge mess again, sorry. How do I back myself out of this one?

@dpsanders
Copy link
Contributor Author

Have now fixed the mess by cherry-picking onto latest master and force pushing.

@dpsanders dpsanders changed the title Fix sincos for non-standard subtypes of Real Fix sincos for reals that are not AbstractFloats Jan 22, 2018
@dpsanders
Copy link
Contributor Author

Are the failures in Circle x86_64 related? If not, then this is ready to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sincos is broken for nonstandard types
8 participants