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 isprimitive bug #108

Merged
merged 2 commits into from
Apr 10, 2022
Merged

fix isprimitive bug #108

merged 2 commits into from
Apr 10, 2022

Conversation

cscherrer
Copy link
Contributor

This PR makes some small changes to isprimitive to work around some errors I was seeing on my local machine. Then again, by my understanding the previous code shouldn't work at all. But it clearly passes the existing tests, so at the very least my understanding is incomplete.

The code in the repo contains the call Core.Compiler.return_type(rrule, (YotaRuleConfig, Ts...,)). This passes a Tuple as the second argument to return_type. I've never seen it work this way - it usually requires a Type{Tuple}. For example, compare

julia> Core.Compiler.return_type(+, (Int, Int))
ERROR: MethodError: no method matching return_type(::typeof(+), ::Tuple{DataType, DataType})
Closest candidates are:
  return_type(::Any, ::DataType) at ~/julia/julia-1.8.0-beta1/share/julia/base/compiler/typeinfer.jl:989
  return_type(::Any, ::DataType, ::UInt64) at ~/julia/julia-1.8.0-beta1/share/julia/base/compiler/typeinfer.jl:995
Stacktrace:
 [1] top-level scope
   @ REPL[2]:1

julia> Core.Compiler.return_type(+, Tuple{Int, Int})
Int64

The repo code also includes the line

Ts = [a isa DataType ? Type{a} : typeof(a) for a in (f, args...)]

This misses the UnionAll case. For example

julia> [a isa DataType ? Type{a} : typeof(a) for a in (Array{Float64},)]
1-element Vector{DataType}:
 UnionAll

I assume it's preferred here to return Type{<:Array{Float64}}. So this PR works instead in terms of a small helper function instance_type, defined as

@inline instance_type(f::F) where {F} = F
@inline instance_type(T::UnionAll) = Type{<:T}
@inline instance_type(T::DataType) = Type{T}

@cscherrer cscherrer changed the title fix is_primitive bug fix isprimitive bug Apr 9, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022

Codecov Report

Merging #108 (78db740) into main (1d73ef5) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   85.35%   85.47%   +0.12%     
==========================================
  Files           9        9              
  Lines         471      475       +4     
==========================================
+ Hits          402      406       +4     
  Misses         69       69              
Impacted Files Coverage Δ
src/cr_api.jl 96.34% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d73ef5...78db740. Read the comment docs.

@dfdx
Copy link
Owner

dfdx commented Apr 9, 2022

That's interesting. What version of Julia do you use? For me both versions work on Julia 1.7.2:

julia> Core.Compiler.return_type(+, (Int, Int))
Int64

julia> Core.Compiler.return_type(+, Tuple{Int, Int})
Int64

@cscherrer
Copy link
Contributor Author

What version of Julia do you use? For me both versions work on Julia 1.7.2

Oh that's interesting, I didn't realize this had changed. I was on Julia 1.8.0-beta1

@dfdx
Copy link
Owner

dfdx commented Apr 10, 2022

Makes sense. So we need it anyways.
Thanks for the PR and thorough explanation of the problem!

@dfdx dfdx merged commit d1b6423 into dfdx:main Apr 10, 2022
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.

3 participants