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

WIP: Hilbert/Banach space structures #27401

Merged
merged 17 commits into from
Jun 19, 2018

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Jun 3, 2018

This is based on the discussion in JuliaLang/LinearAlgebra.jl#496 and is work in progress. It is my first pull request to julia. Thus, please be so kind to help me with improving this PR. Of course, it can also be used as a starting point for more experienced people.

The discussion in JuliaLang/LinearAlgebra.jl#496 seems to have reached a consensus about the naming of norm, vecnorm, and a new opnorm. Basically, norm should replace the old vecnorm, since it is used in most cases to estimate some error or magnitude. Thus, avoiding the cost of the SVD for matrices seems to be good. Moreover, most users are probably not aware of vecnorm and just stick to norm since it is okay in some cases. The old behaviour of norm(A::AbstractMatrix, p), i.e. the computation of the operator/matrix norm, is encoded in opnorm. Jutho has mentioned that he started implementing some similar behaviour in #25093. However, I can not see any commits or changed files there, so this is started from scratch.

Moreover, the behaviour of generic norm functions is changed. Before, it has been along the lines of

norm(x, p) = sum(xi->norm(xi)^p, x)^(1/p).

Since it seems to be more natural (at least for me), I have changed it to

norm(x, p) = sum(xi->norm(xi, p)^p, x)^(1/p).

Thus, given the same coefficients, calling norm on a vector, a matrix or a vector of vectors yields the same result, which seems to be consistent in some sense.

julia> v = [3, -2, 6]
3-element Array{Int64,1}:
  3
 -2
  6

julia> norm(hcat(v,v), 1) == norm(vcat(v,v), 1) == norm([v,v], 1)
true

julia> norm(hcat(v,v), 2) == norm(vcat(v,v), 2) == norm([v,v], 2)
true

julia> norm(hcat(v,v), Inf) == norm(vcat(v,v), Inf) == norm([v,v], Inf)
true

In order to keep norm and dot (or a new inner) consistent, I would also remove vecdot and make dot recursive, analogously to norm, to give a behaviour as norm(x) = sqrt(dot(x,x)). The discussion in JuliaLang/LinearAlgebra.jl#496 about the naming of dot does not seem to have reached a complete consensus. If nobody objected strongly, I would implement the basis inner/dot/scalar product as inner and add const dot = inner as proposed by Stefan Karpinski.

This allows very generic code. One application I have in mind are multi-dimensional PDEs. For example, I would like to discretise a function in a cuboid as right-hand side for a Poisson problem. A straightforward way would be to use coefficients in an b = Array{Float64,3}. Then, I have to solve a linear system A \ b, where A is a linear operator acting as negative Laplacian. If dot and norm work as described above, I could simply use IterativeSolvers.jl for this task, since A is symmetric and positive definite.

@ranocha
Copy link
Member Author

ranocha commented Jun 3, 2018

Note: The new recursive definition of norm gives

julia> using LinearAlgebra, StaticArrays
julia> x = SVector(1, 2); A = @SArray [1 2; 3 4];

julia> xx = [x, x]
2-element Array{SArray{Tuple{2},Int64,1,2},1}:
 [1, 2]
 [1, 2]

julia> AA = reshape([A, zero(A), zero(A), A], (2,2))
2×2 Array{SArray{Tuple{2,2},Int64,2,4},2}:
 [1 2; 3 4]  [0 0; 0 0]
 [0 0; 0 0]  [1 2; 3 4]

julia> norm(xx, 1)
6.0

julia> norm(reinterpret(eltype(x), xx), 1)
6.0

while the old definition gives

julia> norm(xx, 1)
4.47213595499958

julia> norm(reinterpret(eltype(x), xx), 1)
6.0

@ranocha
Copy link
Member Author

ranocha commented Jun 3, 2018

I have added a recursive definition of inner that should be compatible to norm.

In order to get the old non-recursive definition of dot(x,y) = sum(x[i]'*y[i] for i = 1:length(x)) with or without conjugation, the two functions dotc (conjugated) and dotu (unconjugated) could be added, similar to the respective BLAS function names. With these functions, a dotu or dotc product of a vector of matrices and a vector would return a matrix as in the use case of Pauli matrices described by Jutho in JuliaLang/LinearAlgebra.jl#496.

@ranocha
Copy link
Member Author

ranocha commented Jun 3, 2018

Currently, setting const dot = inner and export dot in LinearAlgebra.jl (https://github.com/JuliaLang/julia/pull/27401/files#diff-09308db1337132dff7bb30f366ff7743R373) breaks compilation. I get some error during bootstrap: LoadError("sysimg.jl", 213, ... that I do not fully understand.

@Sacha0
Copy link
Member

Sacha0 commented Jun 4, 2018

Much thanks for taking a shot at these changes @ranocha, and welcome to JuliaLang/julia! Superlative first contribution :). Apologies in advance for a bit of review latency: I hope to eke out review time over the next few evenings. Best!

@ranocha
Copy link
Member Author

ranocha commented Jun 4, 2018

Thank you, @Sacha0 :) I won't possibly have as much time for this in the next days as I would like to have, but I will implement also some versions of dotc and dotu - unless someone is faster than me, of course.

I've fixed the error from OffsetArray by replacing length(x) with length(LinearIndices(x)), as suggested in the devdocs.

@stevengj
Copy link
Member

stevengj commented Jun 5, 2018

I don't think we should have dotc. dot (or inner) should be conjugating (i.e. an inner product), as I think was the consensus in JuliaLang/LinearAlgebra.jl#496.

dotu can be added later — that is a new feature, and can be a separate PR.

@stevengj
Copy link
Member

stevengj commented Jun 5, 2018

norm(x, p) = sum(xi->norm(xi, p)^p, x)^(1/p)

As I think I wrote somewhere in JuliaLang/LinearAlgebra.jl#496, I don't agree with this definition. Although it seems more natural at first glance, it is significantly less general — it requires that norm(xi, p) be defined, not just norm(xi). i.e. it doesn't work if the elements of x are an arbitrary Banach space.

@stevengj
Copy link
Member

stevengj commented Jun 5, 2018

I think it is acceptable to have const dot = inner, though it is not ideal to me. (I'm mainly opposed to having dot and inner be slightly different things.)

@ranocha
Copy link
Member Author

ranocha commented Jun 5, 2018

norm(x, p) = sum(xi->norm(xi, p)^p, x)^(1/p)

As I think I wrote somewhere in JuliaLang/LinearAlgebra.jl#496, I don't agree with this definition. Although it seems more natural at first glance, it is significantly less general — it requires that norm(xi, p) be defined, not just norm(xi). i.e. it doesn't work if the elements of x are an arbitrary Banach space.

I'm very sorry, I did not remember such a comment in JuliaLang/LinearAlgebra.jl#496. Thus, I thought that
norm(x, p) = sum(xi->norm(xi)^p, x)^(1/p)
does not seem to be very convenient for most use cases. In the applications I had in mind,
norm(x, p) = sum(xi->norm(xi, p)^p, x)^(1/p)
would be more useful. However, if the old definition should be used since it is more general, I will change it, of course.

Edit: Should there be some possibility to get sum(xi->norm(xi, p)^p, x)^(1/p)?

@ranocha
Copy link
Member Author

ranocha commented Jun 5, 2018

I think it is acceptable to have const dot = inner, though it is not ideal to me. (I'm mainly opposed to having dot and inner be slightly different things.)

When setting const dot = inner, I got some problems during compilation:

Currently, setting const dot = inner and export dot in LinearAlgebra.jl (https://github.com/JuliaLang/julia/pull/27401/files#diff-09308db1337132dff7bb30f366ff7743R373) breaks compilation. I get some error during bootstrap: LoadError("sysimg.jl", 213, ... that I do not fully understand.

Could someone please give me a hint what has to be done to set const dot = inner? If dot(x,y) = inner(x,y) is used as implemented now (https://github.com/JuliaLang/julia/pull/27401/files#diff-e5541a621163d78812e05b4ec9c33ef4R739), dot and inner could have slightly different meanings if overloaded by users, which does not seem to be good.

@stevengj
Copy link
Member

stevengj commented Jun 5, 2018

Should there be some possibility to get sum(xi->norm(xi, p)^p, x)^(1/p)?

It's easy to write this reduction yourself in cases where it is really needed. I think the default should be something that is as generic as possible, and in particular which doesn't break on arrays of custom types that only define norm(xi).

Currently, setting const dot = inner and export dot in LinearAlgebra.jl breaks compilation.

I'm guessing that the problem has to do with @deprecate_stdlib dot in base/sysimg.jl. Try removing this line temporarily to see if that eliminates the problem.

@ranocha
Copy link
Member Author

ranocha commented Jun 5, 2018

I'm guessing that the problem has to do with @deprecate_stdlib dot in base/sysimg.jl. Try removing this line temporarily to see if that eliminates the problem.

I have tried this on sunday with make clean && make and it threw the same error. However, using make cleanall && make today was successful on my computer. Should I upload these changes or should'n I modify base/sysimg.jl?

@stevengj
Copy link
Member

stevengj commented Jun 5, 2018

If it's working with make cleanall without changing sysimg.jl, great, just update the PR.

@ranocha
Copy link
Member Author

ranocha commented Jun 6, 2018

If it's working with make cleanall without changing sysimg.jl, great, just update the PR.

It compiles well, but if I run make docs, I get the following error

Building HTML documentation.
ERROR: LoadError: AssertionError: haskey(hashes, uuid)
Stacktrace:
 [1] version_data!(::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}) at /.../julia/usr/share/julia/stdlib/v0.7/Pkg/src/Operations.jl:346
 [2] #instantiate#52(::Nothing, ::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::Function, ::Pkg.Types.Context) at /.../julia/usr/share/julia/stdlib/v0.7/Pkg/src/API.jl:514
 [3] #instantiate#51 at /.../julia/usr/share/julia/stdlib/v0.7/Pkg/src/API.jl:483 [inlined]
 [4] instantiate() at /.../julia/usr/share/julia/stdlib/v0.7/Pkg/src/API.jl:481
 [5] top-level scope
 [6] include at ./boot.jl:314 [inlined]
 [7] include_relative(::Module, ::String) at ./loading.jl:1071
 [8] include(::Module, ::String) at ./sysimg.jl:29
 [9] exec_options(::Base.JLOptions) at ./client.jl:267
 [10] _start() at ./client.jl:427
in expression starting at /.../julia/doc/make.jl:6
Makefile:37: recipe for target 'html' failed
make[1]: *** [html] Error 1
Makefile:88: recipe for target 'docs' failed
make: *** [docs] Error 2

I haven't seen this error before. I will try to see what's happening.

@@ -174,7 +174,7 @@ function (-)(J::UniformScaling, A::AbstractMatrix)
end

inv(J::UniformScaling) = UniformScaling(inv(J.λ))
norm(J::UniformScaling, p::Real=2) = abs(J.λ)
opnorm(J::UniformScaling, p::Real=2) = opnorm(J.λ, p)
Copy link
Member

Choose a reason for hiding this comment

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

Why not abs(J.λ)? This should be correct for all p since J.λ is a scalar, no? (Do we even have an opnorm method for scalars?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought someone might want to define some unusual number system with a strange norm as abs that is suited to the specific use case but does not yield a Banach algebra. But this might be rather unusual, so I could replace opnorm with abs. There is method opnorm(x::number, p::Real = 2) in https://github.com/JuliaLang/julia/pull/27401/files#diff-e5541a621163d78812e05b4ec9c33ef4R559.

Copy link
Member

Choose a reason for hiding this comment

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

It is hard for me to imagine such an algebra, and norm(x::Number) defaults to abs(x) anyway, but I guess it doesn't matter as long as the opnorm method is defined, since performance is not critical here.


For any iterable containers `x` and `y` (including arrays of any dimension) of numbers (or
any element type for which `dot` is defined), compute the Euclidean dot product (the sum of
`dot(x[i],y[i])`) as if they were vectors.
any element type for which `inner` is defined), compute the inner product (or dot product
Copy link
Member

Choose a reason for hiding this comment

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

"Euclidean inner product" is perhaps more descriptive. (As opposed to some weighted inner product.)

Missing a close paren after "or scalar product".

Copy link
Member Author

Choose a reason for hiding this comment

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

For me and the professors who have taught me linear algebra, Euclidean inner product implies that the corresponding field are the real numbers. Especially, if a vector space over the complex numbers is considered, the inner product has been called unitary inner product. But if Euclidean inner product means just an inner product in (american) English, I can of course rename it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The close parenthesis is after "or scalar product, i.e. the sum of inner(x[i],y[i]))". If it might be better to have this part of the additional explanation outside of the parentheses, I will of course change it.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for the qualifier "Euclidean" or similar is to distinguish it from some other weighted inner product. But since we write that it is the sum of inner(x[i],y[i]) explicitly, I guess we don't need any other qualifiers.

Yes, I would suggest having the i.e. the sum of ... part go outside of the parentheses, as this is necessary to precisely define what inner product we are using (as opposed to just a synonym).

Copy link
Member

Choose a reason for hiding this comment

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

The docstring should also mention: dot(x,y) and x ⋅ y (where can be typed by tab-completing \cdot in the REPL) are synonyms for inner(x,y).

@Jutho
Copy link
Contributor

Jutho commented Jun 6, 2018

norm(x, p) = sum(xi->norm(xi, p)^p, x)^(1/p)

As I think I wrote somewhere in JuliaLang/LinearAlgebra.jl#496, I don't agree with this definition. Although it seems more natural at first glance, it is significantly less general — it requires that norm(xi, p) be defined, not just norm(xi). i.e. it doesn't work if the elements of x are an arbitrary Banach space.

It's also the first time I hear this argument I think. If the elements xi are from some Banach space with only norm(xi) defined, does it make sense to compute norm(x,p) of a vector x containing such elements xi with p != 2.

@stevengj
Copy link
Member

stevengj commented Jun 6, 2018

If the elements xi are from some Banach space with only norm(xi) defined, does it make sense to compute norm(x,p) of a vector x containing such elements xi with p != 2.

Yes? I'm not sure what you mean by "makes sense". The operation is well defined and is distinct from norm(x), whereas the previous alternative was not defined.

It is also not unreasonable to me — if you are using a vector of vectors as a data structure, it's not crazy to treat each subvector as a "unit" for purposes of e.g. an Linf norm.

Of course, in cases where norm(xi,p) also exists, there are infinitely many other possible meanings of norm(x,p), and in some applications you may want a different choice. But we have to pick something, I think it is reasonable for the default to be a definition that is as generic as possible.

@ranocha
Copy link
Member Author

ranocha commented Jun 6, 2018

On Travis, the following error occurs:

Error in testset OldPkg/pkg:
Error During Test at /tmp/julia/share/julia/test/testdefs.jl:19
  Got exception LoadError("/tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl", 56, KeyError("SparseArrays")) outside of a @test
  LoadError: KeyError: key "SparseArrays" not found
  Stacktrace:
   [1] getindex at ./dict.jl:487 [inlined]
   [2] prune_versions(::Dict{String,OldPkg.Types.VersionSet}, ::Dict{String,Dict{VersionNumber,OldPkg.Types.Available}}, ::Dict{AbstractString,OldPkg.Types.ResolveBacktraceItem}) at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/query.jl:421
   [3] prune_dependencies at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/query.jl:586 [inlined]
   [4] resolve(::Dict{String,OldPkg.Types.VersionSet}, ::Dict{String,Dict{VersionNumber,OldPkg.Types.Available}}, ::Dict{String,Tuple{VersionNumber,Bool}}, ::Dict{String,OldPkg.Types.Fixed}, ::Dict{String,VersionNumber}, ::Set{String}) at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/entry.jl:508
   [5] resolve at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/entry.jl:487 [inlined] (repeats 5 times)
   [6] JuliaLang/julia#3 at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/dir.jl:33 [inlined]
   [7] cd(::getfield(OldPkg.Dir, Symbol("##3#6")){Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}},typeof(OldPkg.Entry.resolve),Tuple{}}, ::String) at ./file.jl:96
   [8] JuliaLang/julia#2 at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/dir.jl:33 [inlined]
   [9] withenv(::getfield(OldPkg.Dir, Symbol("##2#5")){Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}},typeof(OldPkg.Entry.resolve),Tuple{},String}, ::Pair{String,String}) at ./env.jl:148
   [10] #cd#1(::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::Function, ::Function) at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/dir.jl:32
   [11] cd at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/dir.jl:25 [inlined]
   [12] resolve at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/OldPkg/src/OldPkg.jl:233 [inlined]
   [13] (::getfield(Main.Test58Main_OldPkg_pkg.PkgTests, Symbol("##6#7")){Bool,getfield(Main.Test58Main_OldPkg_pkg.PkgTests, Symbol("##8#55")),String,Bool})() at /tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl:37
   [14] withenv(::getfield(Main.Test58Main_OldPkg_pkg.PkgTests, Symbol("##6#7")){Bool,getfield(Main.Test58Main_OldPkg_pkg.PkgTests, Symbol("##8#55")),String,Bool}, ::Pair{String,String}) at ./env.jl:148
   [15] #temp_pkg_dir#5(::Bool, ::Function, ::getfield(Main.Test58Main_OldPkg_pkg.PkgTests, Symbol("##8#55")), ::String, ::Bool) at /tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl:31
   [16] temp_pkg_dir(::Function, ::String, ::Bool) at /tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl:31 (repeats 2 times)
   [17] top-level scope
   [18] include at ./boot.jl:314 [inlined]
   [19] include_relative(::Module, ::String) at ./loading.jl:1071
   [20] include(::Module, ::String) at ./sysimg.jl:29
   [21] include(::String) at /tmp/julia/share/julia/test/testdefs.jl:13
   [22] macro expansion at /tmp/julia/share/julia/test/testdefs.jl:22 [inlined]
   [23] macro expansion at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/Test/src/Test.jl:1080 [inlined]
   [24] macro expansion at /tmp/julia/share/julia/test/testdefs.jl:21 [inlined]
   [25] macro expansion at ./util.jl:289 [inlined]
   [26] top-level scope at ./<missing>:19
   [27] eval at ./boot.jl:316 [inlined]
   [28] #runtests#3(::UInt128, ::Function, ::String, ::String, ::Bool) at /tmp/julia/share/julia/test/testdefs.jl:25
   [29] #runtests at ./<missing>:0 [inlined] (repeats 2 times)
   [30] (::getfield(Distributed, Symbol("##112#114")){Distributed.CallMsg{:call_fetch}})() at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/Distributed/src/process_messages.jl:269
   [31] run_work_thunk(::getfield(Distributed, Symbol("##112#114")){Distributed.CallMsg{:call_fetch}}, ::Bool) at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/Distributed/src/process_messages.jl:56
   [32] macro expansion at /home/travis/build/JuliaLang/julia/usr/share/julia/stdlib/v0.7/Distributed/src/process_messages.jl:269 [inlined]
   [33] (::getfield(Distributed, Symbol("##111#113")){Distributed.CallMsg{:call_fetch},Distributed.MsgHeader,Sockets.TCPSocket})() at ./task.jl:257
  in expression starting at /tmp/julia/share/julia/stdlib/v0.7/OldPkg/test/pkg.jl:56
ERROR: LoadError: Test run finished with errors
in expression starting at /tmp/julia/share/julia/test/runtests.jl:59

Locally, there have been no errors for me. I don't think I might have changed something that could cause the error above. Could someone please give me a hint?

@stevengj
Copy link
Member

stevengj commented Jun 6, 2018

I'm seeing a similar KeyError in Travis for lots of recent PRs (e.g. #27457), so I think that is probably a bug in master — not your fault. Hopefully it will get fixed soon, though I don't see an issue for it yet.

@Jutho
Copy link
Contributor

Jutho commented Jun 6, 2018

Yes? I'm not sure what you mean by "makes sense". The operation is well defined and is distinct from norm(x), whereas the previous alternative was not defined.

Fair enough, but indeed, when it is just a vector of vectors, it is not compatible with the interpretation of this behaving like a block matrix (or block vector in this case). So I guess this need to be documented properly.

With this argument, you also implicitly answered a question I posed in JuliaLang/LinearAlgebra.jl#496: Is the specifier p in norm part of the required interface and should a custom type support a p argument if it wants to extend norm, even when it only allows p=2 (so it should check and throw an error otherwise)? Or could a custom type only implement norm(x) without second argument?

From your reasoning above, I conclude that the second behaviour is just fine.

@ranocha
Copy link
Member Author

ranocha commented Jun 7, 2018

Thank you very much for your comments. I have updated the docstrings accordingly.

@ranocha
Copy link
Member Author

ranocha commented Jun 7, 2018

There is a const INNER_CUTOFF (previously const DOT_CUTOFF) in https://github.com/JuliaLang/julia/pull/27401/files#diff-589036f8d44fe17bc30f55c356a8ea06R8 that does not seem to be used anywhere else. Should there be an implementation of inner similar to the one of norm (switching between BLAS and generic version depending on the length as in https://github.com/JuliaLang/julia/pull/27401/files#diff-589036f8d44fe17bc30f55c356a8ea06R142) or should INNER_CUTOFF be removed?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jun 7, 2018

Should we just get rid of dot and while we're at this and just make inner(u, v) the one and only way to spell this for now? Someone can add support for ⟨u, v⟩ as a syntax for this in the future.

@@ -529,15 +529,15 @@ end
diff(a::SparseMatrixCSC; dims::Integer) = dims==1 ? sparse_diff1(a) : sparse_diff2(a)

## norm and rank
vecnorm(A::SparseMatrixCSC, p::Real=2) = vecnorm(view(A.nzval, 1:nnz(A)), p)
norm(A::SparseMatrixCSC, p::Real=2) = norm(view(A.nzval, 1:nnz(A)), p)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually need a view here?

Copy link
Member

Choose a reason for hiding this comment

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

I think so… we don't want to make a copy of A.nzval[1:nnz(A)].

Copy link
Contributor

Choose a reason for hiding this comment

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

but why not use A.nzval directly?

Copy link
Member

Choose a reason for hiding this comment

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

The A.nzval might be longer than nnz(A).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks. Can that happen without manually messing with A.nzval?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so

Copy link
Member

Choose a reason for hiding this comment

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

I thought it could happen with .= assignment to sparse matrices, but I don't remember clearly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the number of entries not matching the storage length can come about through e.g. broadcast.

@stevengj
Copy link
Member

stevengj commented Jun 7, 2018

@StefanKarpinski, it's not a huge deal to me, but I think it would be a shame to lose the infix notation and the familiar name to people coming from the physical sciences. On the other hand, I can see the argument for only having a single name for this function.

@ranocha
Copy link
Member Author

ranocha commented Jun 19, 2018

Thanks, @Sacha0. I've rebased on master.

@andreasnoack andreasnoack merged commit d128b9c into JuliaLang:master Jun 19, 2018
@jebej
Copy link
Contributor

jebej commented Jun 19, 2018

Awesome job @ranocha!

@ranocha
Copy link
Member Author

ranocha commented Jun 19, 2018

Thanks!

@andreasnoack
Copy link
Member

The change to dot was just brought up on Discourse, https://discourse.julialang.org/t/unexpected-result-of-multiplication-with-adjoint/21447. For the record here, I think the change to dot was unfortunate since it makes vectors and one-columns matrices behave differently and it disallows representing a matrix as a vector of rowvectors, i.e.

julia> X1 = randn(3,2);

julia> X2 = [X1[i,:]' for i in 1:size(X1, 1)];

julia> X3 = reshape(X2, length(X2), 1);

julia> X1'X1
2×2 Array{Float64,2}:
 2.58747   0.733045
 0.733045  0.420716

julia> X2'X2
3.0081843634412744

julia> (X3'X3)[1]
2×2 Array{Float64,2}:
 2.58747   0.733045
 0.733045  0.420716

@StefanKarpinski
Copy link
Member

What do you propose as a way forward, @andreasnoack?

@andreasnoack
Copy link
Member

We can't do anything before have we have versioned standard libraries or Julia 2.0 so, for now, it's just for the record. However, once we can adjust things, I'd prefer that dot didn't flatten the result such X2'X2 would give the same result as (X3'X3)[1].

@Jutho
Copy link
Contributor

Jutho commented Mar 4, 2019

I still think dot should produce a scalar and thus be recursive, but when I want dot, I write dot(a,b), not a'b.

Ignorantly extrapolating my own coding style :-), maybe a'b should be a different function, a non-recursive dot, so that they only coincide when a and b are Vector{<:Number}. This would also allow people to write things like v'σ if σ is a vector of Pauli matrices and v a vector of numbers. Recursiveness could become a keyword argument to dot, but I would still prefer the default value to be recursive=true for dot(a,b), but maybe a'b can then call dot(a,b; recursive=false).

@oschulz
Copy link
Contributor

oschulz commented Nov 7, 2019

I just ran into this in the context of JuliaStats/StatsBase.jl#534 and JuliaStats/StatsBase.jl#518. I'm not sure if I caught all of the above (and related issues), but I'm curious - might there be a non-recursive variant of dot with dot_nonrec(A, x) ≈ sum(A .* x) semantics in the future?

@ranocha
Copy link
Member Author

ranocha commented Nov 8, 2019

There is an open PR for such a function, but it has been mostly rejected because of doubts regarding naming and usefulness.

@oschulz
Copy link
Contributor

oschulz commented Nov 8, 2019

Thanks for the link!

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.