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.

Sign up for GitHub

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

Jump to bottom

Special case empty covec-diagonal-vec product #35557

Merged
merged 10 commits into from
Apr 25, 2020
Merged

Special case empty covec-diagonal-vec product #35557

merged 10 commits into from
Apr 25, 2020

Conversation

MasonProtter
Copy link
Contributor

Closes #35424

MasonProtter and others added 3 commits April 22, 2020 17:50
@MasonProtter @tkf
Update stdlib/LinearAlgebra/src/diagonal.jl
7910d08 
Co-Authored-By: Takafumi Arakaki <takafumi.a@gmail.com>
@MasonProtter @tkf
Update stdlib/LinearAlgebra/src/diagonal.jl
df36bf9 
Co-Authored-By: Takafumi Arakaki <takafumi.a@gmail.com>
@MasonProtter
fix
310abcd
@tkf
Copy link
Member

tkf commented Apr 23, 2020

Oops my suggestion introduced a bug:

Error During Test at /buildworker/worker/tester_linux64/build/share/julia/stdlib/v1.5/LinearAlgebra/test/diagonal.jl:643
  Test threw exception
  Expression: y' * D * y == (y' * D) * y == (y' * A) * y
  MethodError: no method matching zero(::Type{AbstractArray{Int64,N} where N})
  Closest candidates are:
    zero(!Matched::Type{Pkg.Resolve.FieldValue}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/Resolve/fieldvalues.jl:38
    zero(!Matched::Type{LibGit2.GitHash}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/LibGit2/src/oid.jl:220
    zero(!Matched::Type{Missing}) at missing.jl:103
    ...
  Stacktrace:
   [1] *(::LinearAlgebra.Adjoint{LinearAlgebra.Adjoint{Int64,Array{Int64,1}},Array{Array{Int64,1},1}}, ::LinearAlgebra.Diagonal{Array{Int64,2},Array{Array{Int64,2},1}}, ::Array{Array{Int64,1},1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/LinearAlgebra/src/diagonal.jl:663
   [2] top-level scope at /buildworker/worker/tester_linux64/build/share/julia/stdlib/v1.5/LinearAlgebra/test/diagonal.jl:643
   [3] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1114
   [4] top-level scope at /buildworker/worker/tester_linux64/build/share/julia/stdlib/v1.5/LinearAlgebra/test/diagonal.jl:635

https://build.julialang.org/#/builders/22/builds/11086/steps/5/logs/stdio

Indeed the dot product should work even with non-concrete element types like y = Any[ones(2, 2)] as long as the input is not empty. We can't use zero in this case. Sorry about that.

@MasonProtter
Copy link
Contributor Author

Makes sense! Nothing to apologize for, I didn't anticipate the error either.

dkarrasch
dkarrasch requested changes Apr 23, 2020
View reviewed changes
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

The dot issue was probably just oversight, but the difference between the two * methods and dot is relevant, and must remain.

stdlib/LinearAlgebra/src/diagonal.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/diagonal.jl Outdated Show resolved Hide resolved
@dkarrasch dkarrasch added the domain:linear algebra Linear algebra label Apr 23, 2020
@MasonProtter @dkarrasch
Update stdlib/LinearAlgebra/src/diagonal.jl
b53e804 
Co-Authored-By: Daniel Karrasch <daniel.karrasch@posteo.de>
@tkf
Copy link
Member

tkf commented Apr 24, 2020

the difference between the two * methods and dot is relevant

I totally missed it. Thank you!

@MasonProtter
Copy link
Contributor Author

I don't think these CI failures are related.

@dkarrasch dkarrasch merged commit 1a9925d into JuliaLang:master Apr 25, 2020
mbauman added a commit that referenced this pull request Apr 28, 2020
@mbauman
Merge remote-tracking branch 'origin/master' into ajf/view-of-range
7387f65 
* origin/master: (833 commits)
  Improve typesubtract for tuples (#35600)
  Make searchsorted*/findnext/findprev return values of keytype (#32978)
  fix buggy rand(RandomDevice(), Bool) (#35590)
  remove `Ref` allocation on task switch (#35606)
  Revert "partr: fix multiqueue resorting stability" (#35589)
  exclude types with free variables from `type_morespecific` (#35555)
  fix small typo in NEWS.md (#35611)
  enable inline allocation of structs with pointers (#34126)
  SparseArrays: Speed up right-division by diagonal matrices (#35533)
  Allow hashing 1D OffsetArrays
  NEWS item for introspection macros (#35594)
  Special case empty covec-diagonal-vec product (#35557)
  [GCChecker] fix a few tests by looking through casts
  Use norm instead of abs in generic lu factorization (#34575)
  [GCChecker,NFC] run clang-format -style=llvm
  [GCChecker] fix tests and add Makefile
  Add introspection macros support for dot syntax (#35522)
  Specialize `union` for `OneTo` (#35577)
  add pop!(vector, idx, [default]) (#35513)
  bump Pkg version (#35584)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkf tkf tkf left review comments

@dkarrasch dkarrasch dkarrasch approved these changes

Assignees
No one assigned
Labels
domain:linear algebra Linear algebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adjoint times Diagonal times Vector for n = 0
3 participants
@MasonProtter @tkf @dkarrasch