-
-
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
Restrict +,- and / to algebraic operations (for matrices). Use . versions for elementwise operations. Add UniformScaling matrix. Was:Don't let division with array default to elementwise division. #5810
Conversation
There is a test whose behavior needs to be changed too. |
My inclination would be to not define |
Please don't do this. |
Agree with not defining |
I'd be okay with removing
|
I have tried to follow the suggestions in #5807. So now you can do
and
but get
I have defined During the adjustment of code in base and test I removed a few lines in |
Any change of behavior should not be silent. The behavior of |
@@ -0,0 +1,49 @@ | |||
import Base: getindex, show, +, * | |||
immutable IdentityMatrix{T<:Number} <: AbstractMatrix{T} |
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.
It still seems weird to me to call this IdentityMatrix
because of the scale factor. Maybe ScaledIdentity
or ScalingOperator
?
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.
Ah. I forgot to mention that. I remember you comment in #5807 and I agree that the name IdentityMatrix
is not precise because of the λ
, but couldn't come up with a better name. I think ScalingOperator/Matrix
is used for a matrix that allow distinct diagonal elements. ScaledIdentity
is fine.
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.
+1 for ScaledIdentity
.
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.
@StefanKarpinski do you have any suggestion of this name?
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.
ScaledIdentity
seems reasonable.
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.
What about Homothety ? Maybe it's just in french but it is quite common in math in my experience.
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've never heard that word before. Having looked it up, this isn't actually what it means – it's an affine transformation that this would be the non-translation part of. In other words, this is a homethety with zero translation. In any case, I think it best not to use that term.
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.
You are right of course, but usually it is implied to be a linear homothety in a linear algebra context. Anyway, if it is not familiar in english it's not worth it at all - apart from being nice to the ear :-)
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.
French wikipedia defines it without the translation, so we could call it Homothétie
, but I am not sure it is the best PR strategy for the new type. English wikipedia suggests the name UniformScaling
.
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 like UniformScaling
best so far. There's something a bit oxymoronic about ScaledIdentity
, after all.
@stevengj Thank you for the comments. I have updated the pr. Changes:
|
import Base: getindex, showarray, +, -, *, /, ctranspose, transpose | ||
import Base.LinAlg: SingularException | ||
immutable UniformScaling{T<:Number} <: AbstractMatrix{T} | ||
λ::T |
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.
Looks like this source file uses tab for indentation. I think the Julia base uses 4-spaces.
Btw, thanks for taking the lead on this! |
Ok. I have changed matrix to operator in the documentation and tabs to spaces in @toivoh You're welcome. I agree that it would be very useful if the abstractions were documented. There has been several issues related to different perceptions of abstractions. |
I think we should merge this as soon as possible, preferably before 0.3 release. |
+1 |
Looks good to me. |
…operator I. Let Number/Matrix be the inverse. Update docs.
Restrict +,- and / to algebraic operations (for matrices). Use . versions for elementwise operations. Add UniformScaling matrix.
I wasn't sure if this was a 0.3 thing, but now it is. |
@@ -189,6 +189,16 @@ export PipeString | |||
@deprecate svdfact(A,thin) svdfact(A,thin=thin) | |||
@deprecate svdfact!(A,thin) svdfact(A,thin=thin) | |||
@deprecate svd(A,thin) svd(A,thin=thin) | |||
@deprecate (+)(A::Array{Bool},x::Bool) A .+ x |
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.
These deprecations will have to be replaced with a gentle error explaining why you can't do [1:10] + 4
, when the deprecation period is over. A NoMethodError
will be confusing and cause people to submit PRs with the "missing" methods.
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'm with @andreasnoackjensen on this one. |
The place I've found it most annoying is when doing vector + scalar, where arguably, there is no ambiguity. |
Agreed. In that case you don't gain anything by forcing users to use |
I don't like having to type |
You don't have to type (And to the purist argument that "linear algebra doesn't define matrix + scalar", I would say that (a) Julia has never been restricted to solely the operations of linear algebra, (b) math doesn't prevent you from defining whatever notation you want, and (c) the notation array+scalar has already been defined and is widespread in scientific computing. The only question is whether it is confusing, but the widespread adoption of array+scalar means that not defining it is far more confusing.) |
Yes, but I thought before this change |
I don't have strong preferences either way. If we restore the original behavior, then there would be some subtle differences between |
@BobPortmann, no, before this change, |
@lindahua, couldn't you make exactly the same argument for The point is simply that |
@stevengj Yes, of course. I meant "array broadcasting" (i.e., modifying an array to match another array). "Scalar broadcasting" seems so natural I don't even think of it as broadcasting (although it is). I'm fine with relaxing the rules for array + scalar, but really don't think it matters much. Once you get in the habit of typing all those annoying |
@BobPortmann, as made concrete in #7226, the only controversy here is the behavior for array+scalar. No one is proposing that array+array work unless the arrays have the same shape. |
@GaborOszlanyi, I think that a couple of months ago, many of us felt that Now that I've had a chance to use it, I'm sad to say that Julia should be fun. Being nagged about pointless algebraic purity is not fun. |
Language design, turns out to be a subfield of psychology, not mathematics – it's highly experimental – sometimes you've just got to try things out and see if they work or not. Even after a few months, this is still annoying. |
Should we try enabling it for vectors only to see how that feels? |
Seems too fiddly. I'm inclined to call this a failed experiment, re-allow scalar + array and be done with it. |
With all due respect to theoretical purity of the distinction between 2, I prefer the old behavior, so I guess thats another 👍. I still forget it every time... |
Considering the amount of accidental pain that broadcasting in NumPy has caused me over the years I really like the current clear distinction in Julia. When working with arrays the only "type-safety" that you have are the dimensions of your arrays and whether or not something is a scalar, so I'll gladly take any additional safety that I can get. |
I do like the clear separation between broadcasting and algebraic operations as @andreasnoackjensen and @johnmyleswhite suggest. I also like the fact that the accidental pain of broadcasting as in Numpy is avoided as @ninjin says. But, simultaneously, I really hate doing I personally prefer that we only allow |
Yes, please allow scalar + array. I use it all the time and for existing code the only feasible way was to override the deprecations and bring back scalar + array myself. For new code I've been using .+ and it has been a pain. |
It looks like the consensus leans in favor of bringing back |
Define Number rhs for Matrix lhs when dimensions match.
This is a followup on the discussion in #5807.