-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
recursive vecdot and vecnorm #25093
recursive vecdot and vecnorm #25093
Conversation
I don't know, it seems like we should have On the other hand, I can imagine cases where "recursive Remind me how this is connected to recursive |
Let me in this comment only address the case of
I don't think in math there is a notion of The problem is that in 0.7 there is no way of getting a scalar inner product out of vectors filled with matrices. For in 0.6, |
Yes I just stumbled upon that PR, together with #22392. I agree that |
Any further thoughts or input on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to use vecnorm
and vecdot
such that e.g.
julia> x = rand(1:10, 4, 2);
julia> y = [x[1:2,1:2], x[3:4,1:2]];
julia> vecdot(x,x) == vecdot(y,y)
This, of course, raises the question if vec
should be recursive but let's leave that discussion for another issue.
I agree that a recursive I'm very skeptical of making Note that you'll need to update the docstrings too. |
This is the one the lessons from making
I think the "normal" argument here is also very similar to our discussions of recursive I think I'm in favor of recursive |
That's great. Before moving over to the In the existing code, something like |
This is definitely a breaking change. Note that this would also break I feel like recursive vec, vecdot, and vecnorm should be separate functions. vecr? |
I guess you could have |
Thanks for the feedback. Regarding the behaviour: I am correct that we do want to compute a 1-norm or inf-norm (or any p-norm) recursively on the elements right? E.g. for a vector of vectors. How should such a breaking change be implemented? A deprecation cannot handle this, correct? Regarding the implementation: how about a general |
@Jutho, As I said, my inclination is that "recursive |
Ok, just so that I understand, this means essentially status quo for the current implementation of Personally, I am not really convinced by the current implementations. In particular,
|
Also, I don't see the benefit of using I think it's more useful to have a norm that acts on the natural vector space that arises for any family of objects that you can multiply with scalars and that you can add together. That's at least how I always interpreted |
|
julia> v=[randn(2,2),randn(2,2)]
2-element Array{Array{Float64,2},1}:
[1.43478 -1.06198; 0.843557 -0.714887]
[-0.253848 0.532226; 2.03743 -1.05801]
julia> vecnorm(v)
3.146799307329483
julia> hypot(norm(v[1]),norm(v[2]))
3.146799307329483 Will the cost of the intermediate |
Sorry, I was thinking of |
What is the recommended course of action here? Do other contributors to LinAlg and transpose/adjoint contributors, e.g. @andyferris and @Sacha0, have an opinion about the 'correct' behavior of A summary. Current situation:
My proposal:
|
We spent some time on |
Thanks for the link to that discussion. So it seems that the behavior of My main interest is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should change these to be inconsistent with vec
, and I'm not convinced the meaning of vec
should be changed.
I am surprised by that reaction, as, back in #22220, you were arguing that I am also not sure that the current implementation really satisfies |
@Jutho, I'm not arguing that it isn't useful to have a recursive vecdot-like function. Just that it shouldn't be called With regard to your other point, I think it would be reasonable to add a |
This still is the case, for I still don't quite get the argument. For most common types, my proposed For the cases where it would differ, it also differs with the current implementation, and this will not be fixed completely by adding But I don't see the point of a function # 5 methods for generic function "vec":
[1] vec(v::Union{Adjoint{T,#s520}, Transpose{T,#s520}} where #s520<:(AbstractArray{T,1} where T) where T) in Base.LinAlg at linalg/adjtrans.jl:117
[2] vec(x::AbstractSparseArray{Tv,Ti,1} where Ti where Tv) in Base.SparseArrays at sparse/sparsevector.jl:876
[3] vec(rowvec::RowVector) in Base.LinAlg at deprecated.jl:2469
[4] vec(a::AbstractArray{T,1} where T) in Base at abstractarraymath.jl:39
[5] vec(a::AbstractArray) in Base at abstractarraymath.jl:38 most of which are trivial. Furthermore, I don't understand why the generic |
I wasn't suggesting (I also still dislike the change that was made to have
|
I see your point that a recursive It also bugs me that |
Would this be a good place for a |
Thanks for that. I was starting to doubt whether I was able to successfully get my point across.
My argument was already for the current implementation, not my new proposal. Even currently, this extra definition is not sufficient, because of the way
Me too, I liked your arguments in the thread where that was discussed. Because of this, I rather have started using
I would love to see
But Do you think there is any use case where |
I don't think this is a place where there is one right answer – some people have good reasons for one behavior while others have good reasons for the other. The non-recursive behavior seems simpler and more intuitive to me, so I would propose a |
My admittedly somewhat naive impression is that if you're working with, say, vectors of 2x2 invertible matrices – i.e. elements of SO(2) – then your "scalar type" of the vector space is SO(2), so I would expect the norm of such a vector to be an element of SO(2), i.e. a matrix. That would seem to suggest a non-recursive norm rather than the recursive one. |
I don't think SO(2) is correct here, it's GL(2) (genera linear group) but with also addition so that you have a ring. But that aside, I certainly get your point. In fact, I have always liked that kind of interpretation: Julia Base should just defines linear algebra for However, that interpretation is, from my point of view, in conflict with having But, now that I have finally accepted the recursive behavior down to the scalar level, I think the only consistent possibility is for But really, I have no intent to stir up that discussion again. It's good that actual useful work is being done instead of time being invested in these endless discussions about formal mathematical generalizations. Github threads are probably not the right medium for that anyway, and often lead to misunderstanding each other's argument. Unless any of you have a strong opinion about changing the current situation, I will just close this PR and let it rest. |
Yes, thanks for the correction on SO(2) versus GL(2). On the triage call just now, I was just expressing my concern that we have these very similar debates regularly and neither side is demonstrably correct since they're both valid interpretations, but a different random camp wins the debate each time, which leaves the set of behaviors implemented as a pastiche of these different philosophies, so the point you make about recursive |
This is a very interesting discussion. Like @Jutho, I feel that non-recursive behavior ( In #20978 (see #20978 (comment) and #20978 (comment)) we discussed this kind of thing a lot, and in particular there seems no way that that system can "infer" when the user wants their 2x2 submatrices to be identified as a single scalar element of GL(2), or as 4 numbers in a block matrix. This "interpretation" affects the result of many functions like To support block vectors/matrices (of matching sizes) just as we do the flattened version (which we should definitely be doing, or else there's really no point of recursive
AFAICT this PR address all but Can I also point out that |
(Umm... the interface shows I reopened this PR. FYI this was unintentional - I must have clicked the wrong button, sorry. Feel free to close though I do think it would be nice to have a proper resolution here). |
Linalg folks need to come to a decision here. |
Stefan's |
I can try to change my PR but I have a busy week ahead. I am fine either way but would like to link to this discussion which I think is important: #25565 Essentially, my motivation for this PR was that I am interpreting |
On that topic, I’d favor renaming |
After rereading this thread and #22220, I can appreciate arguments for both the existing and proposed definitions of I strongly sympathize, however, with The preceding sympathy derives from my interpretation of Thanks for the lively discussion all! 😄 Best! |
So what are we actually going to do here? There's a strong chance that today's trage will be the last before we cut an alpha release, so if any action is going to happen here, it should be proposed and acted upon post-haste. Otherwise I'm just going to close this issue and remove it from the milestone. |
We have discussed several things in this thread but with #25093 (comment), it seems to me that making |
Mulling this issue over, I sympathize progressively more with the desire for recursive behavior. Perhaps more descriptive names for the proposed operations would be |
Thanks for the comments @Sacha0 and everybody. Please also have a look at #25565. I like the names And as stated in my last comment in #25565, I think that interface would be completed by a corresponding |
I seem to have screwed up this PR by forced pushing latest julia master back to my own fork, forgetting that this PR was living there in the master branch. Unless anybody knows how to undo this, I will prepare a new PR based on the discussion so far. |
For consistency with recursive
adjoint
(and to a lesser extent transpose), I thinkvecdot
andvecnorm
(at least for 2 norm) should work recursively. It already does now, but strangely enough usingdot
andnorm
within the definition ofvecdot
andvecnorm
, respectively.While doing so, I discovered that for
a=[randn(2,2), randn(2,2)]
, the current behaviour ofdot
andvecdot
differs greatly between 0.6.1 and 0.7.We have
dot(a,a)
: matrix in 0.7, scalar in 0.6.1vecdot(a,a)
: error in 0.7, scalar in 0.6.1The underlying reason is that
dot
of two matrices, i.e.dot(a[1], a[1])
is defined in 0.6.1 (beingBLAS.dot
) and yields a scalar, whereas in 0.7 this gives an error, the reason being thatBLAS.dot
is never used anymore (not even for vectors) and the definition ofdot
inlinalg/generic
only acceptsAbstractVector
. Is this intentional? I don't find it in NEWS.md?This PR will currently break tests, because it does also change behavior for
vecnorm(a,p)
withp != 2
. Currently,generic_vecnormInf
and friends just call norm on the elements, whereas this PR again callsvecnorm
with the samep
value. It seems to me that this is what one wants if one of the motivations for the recursive behavior is block matrices. I have no preference about this, but wanted to ask for opinions/thoughts.