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 sparse broadcast macro hygiene issue #18705

Merged
merged 1 commit into from
Oct 4, 2016
Merged

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Sep 27, 2016

Due to a macro hygiene issue introduced in #17302, broadcast methods specialized for unary operations over SparseMatrixCSCs are not being called:

julia> versioninfo()
Julia Version 0.6.0-dev.804
Commit 182f4b3* (2016-09-27 03:28 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin15.5.0)
  CPU: Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas64_
  LIBM: libopenlibm
  LLVM: libLLVM-3.7.1 (ORCJIT, ivybridge)

julia> sin.(spdiagm(1.0:5.0))
5×5 Array{Float64,2}:
 0.841471  0.0       0.0       0.0        0.0
 0.0       0.909297  0.0       0.0        0.0
 0.0       0.0       0.14112   0.0        0.0
 0.0       0.0       0.0      -0.756802   0.0
 0.0       0.0       0.0       0.0       -0.958924

This PR fixes that issue,

julia> sin.(spdiagm(1.0:5.0))
5×5 sparse matrix with 5 Float64 nonzero entries:
        [1, 1]  =  0.841471
        [2, 2]  =  0.909297
        [3, 3]  =  0.14112
        [4, 4]  =  -0.756802
        [5, 5]  =  -0.958924

and introduces tests to guard against regression. Best!

@tkelman tkelman added sparse Sparse arrays bugfix This change fixes an existing bug labels Sep 27, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Sep 29, 2016

Absent objections or requests for time, I plan to merge this Saturday (assuming I can work up the first-merge nerve!). Best!

@vtjnash
Copy link
Member

vtjnash commented Sep 29, 2016

Not an objection, but I'm somewhat surprised we want to 'esc'ape the function name.

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 29, 2016

Not an objection, but I'm somewhat surprised we want to 'esc'ape the function name.

Without escaping the function name, the macro hygiene system replaces the function name with a distinct identifier. Illustration:

julia> macroexpand(:(@oldversion parentmeth childmeth))
quote
    #5#broadcast(::typeof(childmeth),#7#A::SparseMatrixCSC) = begin  # REPL[3], line 4:
            parentmeth(childmeth,#7#A)
        end
end

julia> macroexpand(:(@newversion parentmeth childmeth))
quote
    broadcast(::typeof(childmeth),#9#A::SparseMatrixCSC) = begin  # REPL[4], line 5:
            parentmeth(childmeth,#9#A)
        end
end

So the new method does not attach to the correct function (broadcast). Thoughts? Thanks!

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2016

similar-ish to #18122 ?

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 30, 2016

similar-ish to #18122 ?

Perhaps tenuously? :)

@Sacha0 Sacha0 merged commit 86b1e53 into JuliaLang:master Oct 4, 2016
@Sacha0 Sacha0 deleted the scrubharder branch October 4, 2016 21:58
@tkelman tkelman mentioned this pull request Oct 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants