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

trig functions are not type-stable for complex inputs #14

Closed
waldyrious opened this issue Jan 7, 2017 · 7 comments
Closed

trig functions are not type-stable for complex inputs #14

waldyrious opened this issue Jan 7, 2017 · 7 comments

Comments

@waldyrious
Copy link
Collaborator

waldyrious commented Jan 7, 2017

Came across this while working to improve test coverage:

julia> show(Base.return_types(sintau, Tuple{Complex}))
Any[Any]

julia> show(Base.return_types(costau, Tuple{Complex}))
Any[Any]

julia> show(Base.return_types(modtau, Tuple{Complex}))
Any[Any]
@waldyrious
Copy link
Collaborator Author

julia> sintau(convert(Int,1.0)) === zero(Int)
true

julia> sintau(convert(Float64,1.0)) === zero(Float64)
true

julia> sintau(convert(Complex,1.0)) === zero(Complex)
false

@waldyrious
Copy link
Collaborator Author

Note: this may be helped by JuliaLang/julia#20268

@giordano
Copy link
Member

giordano commented May 9, 2017

I don't think this is an issue:

julia> show(Base.return_types(sintau, Tuple{Complex{Float64}}))
Any[Complex{Float64}]

In addition, in PR #19 I used @inferred in new tests, that takes care of testing type stability of the result.

@waldyrious
Copy link
Collaborator Author

Before going further, I'd like to make sure you're aware of the context of my role in this package. I am currently maintaining it due to my interest in both Julia and tau, and the original author's lack of availability to maintain the package. However, my experience programming Julia is rather superficial, so I probably won't be able to communicate with you via code snippets alone :) -- and I'll likely ask some silly questions, so bear with me.

With that said, I must say I don't completely follow what you mean with your comment above. I'd appreciate if you could expand a bit.

@giordano
Copy link
Member

giordano commented May 14, 2017

Ok, don't worry ;-)

I don't know exactly what Base.return_types is supposed to do, it isn't documented, I can only guess based on its name. I simply showed that using Complex{Float64} as input type (that is, a Complex object with the parameter explicitly specified), the test is fine (and I added such test at https://github.com/JuliaMath/Tau.jl/pull/19/files#diff-fce720c43af3c52c862fd7451c7374b8R53). Instead, you were using Complex without specifying the parameter, that is the collection of all Complex types, and you can't infer a single returned type for all these input types.

In addition, in the same PR I used everywhere @inferred, which is very useful to test type stability of the given expression. If the function wrapped by @inferred isn't type-stable, an error is raised.

In the end, I believe this issue can be closed, as it isn't an issue at all ;-)

@waldyrious
Copy link
Collaborator Author

Thanks for the details! I had never really studied how Complex works at sufficient detail, which caused my confusion. I assumed it was a straightforward concrete type like Int, while indeed it is more of a container type (like an array of numbers, I suppose).

To confirm that I got this right: the correct form of the Complex test in the second above would be

julia> sintau(convert(Complex,1.0)) === zero(Complex{Float64})
true

right?

In any case, yes, let's close this.

@giordano
Copy link
Member

Exact! Indeed

julia> zero(Complex)
0 + 0im

julia> typeof(ans)
Complex{Int64}

julia> sintau(one(Complex))
0.0 + 0.0im

julia> typeof(ans)
Complex{Float64}

Thus, sintau(one(Complex)) and zero(Complex) have different types, and === (which tests perfect equality) necessarily returns false.

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

No branches or pull requests

2 participants