-
-
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
deprecate expm in favor of exp #23233
Conversation
NEWS.md
Outdated
@@ -296,6 +296,8 @@ Deprecated or removed | |||
full path if you need access to executables or libraries in the `JULIA_HOME` directory, e.g. | |||
`joinpath(JULIA_HOME, "7z.exe")` for `7z.exe` ([#21540]). | |||
|
|||
* `expm` has been deprecated in favor of `exp` ([#CATS]). |
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.
🐈 🐈
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.
The best PR number :). Fixed on push. Thanks!
base/deprecated.jl
Outdated
@deprecate expm(A::Hermitian) exp(A) | ||
@deprecate expm(A::Symmetric) exp(A) | ||
@deprecate expm(D::Diagonal) exp(D) | ||
@deprecate expm(x::Number) exp(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.
Unless we're only deprecating a few methods of expm
, you should be able to just do
@deprecate expm! exp!
@deprecate expm exp
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 kept the signatures narrow out of concern for clobbering expm[!]
definitions outside Base. Should we not worry about that? If not, then agreed, the simpler/wider form would be better :).
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.
Those functions simply won't exist in Base
in the future so external packages would need a deprecation warning for a function deprecation. It's up to them if they want to define and export expm
themselves, or switch to Base.exp
.
base/deprecated.jl
Outdated
@@ -220,7 +220,7 @@ for f in (:sin, :sinh, :sind, :asin, :asinh, :asind, | |||
:tan, :tanh, :tand, :atan, :atanh, :atand, | |||
:sinpi, :cosc, :ceil, :floor, :trunc, :round, | |||
:log1p, :expm1, :abs, :abs2, | |||
:log, :log2, :log10, :exp, :exp2, :exp10, :sinc, :cospi, | |||
#=:log,=# :log2, :log10, #=:exp,=# :exp2, :exp10, :sinc, :cospi, |
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.
Is removing log
here related to the expm
->exp
change? Also perhaps better to delete than to comment out?
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.
The :log
removal snuck in from another commit, and I will delete :exp
rather than comment it out as you prefer. Fixed on push. Thanks! :)
Looks like there are |
base/linalg/dense.jl
Outdated
|
||
# Matrix exponential | ||
|
||
""" | ||
expm(A) | ||
exp(A) |
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.
Perhaps worth adding ::AbstractMatrix
here?
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.
Good call! Fixed on push. Thanks!
base/deprecated.jl
Outdated
@@ -1632,6 +1632,15 @@ function SymTridiagonal(dv::AbstractVector{T}, ev::AbstractVector{S}) where {T,S | |||
SymTridiagonal(convert(Vector{R}, dv), convert(Vector{R}, ev)) | |||
end | |||
|
|||
# deprecate expm in favor of exp | |||
@deprecate expm!(A::StridedMatrix{<:LinAlg.BlasFloat}) exp!(A) |
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.
Would the following be enough?
@deprecate expm exp
@deprecate expm! exp!
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 kept the signatures narrow out of concern for clobbering expm[!]
definitions outside Base. Should we not worry about that? If not, then agreed, the simpler/wider form would be better :).
base/linalg/linalg.jl
Outdated
@@ -9,7 +9,7 @@ import Base: USE_BLAS64, abs, big, broadcast, ceil, conj, convert, copy, copy!, | |||
ctranspose, eltype, eye, findmax, findmin, fill!, floor, full, getindex, | |||
hcat, imag, indices, inv, isapprox, isone, IndexStyle, kron, length, map, | |||
ndims, oneunit, parent, power_by_squaring, print_matrix, promote_rule, real, round, | |||
setindex!, show, similar, size, transpose, trunc, typed_hcat | |||
setindex!, show, similar, size, transpose, trunc, typed_hcat, exp |
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 is in alphabetic order, perhaps move exp
to the right position? 😄
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.
Good catch! Fixed on push. Thanks!
Seems like a good idea if MATLAB and numpy weren't a thing. It seems way too error prone and confusing for new users. |
Good catch! Fixed on push. Thanks! |
5228f10
to
367d019
Compare
Very awesome! I strongly support. @yuyichao I think people will be able to learn pretty quickly - it's more in line with the rest of |
When I first read this, I thought you were advocating for the downfall of MATLAB and numpy. |
I highly doubt that. I've tried in the past months ever since it was proposed and no I can't stop expecting |
Also, such a mistake is simply too easy to make. I can't count the number of times I accidentally passed an array to a function forgetting to add the So again, I'll be OK with this change if the function doesn't give an error but the result will do in most cases but that's simply not the case. |
All exported names in Base ending in julia> filter(x->endswith(String(x), "m") && Symbol(chop(String(x))) in names(Base), names(Base))
6-element Array{Symbol,1}:
:diagm
:expm
:logm
:sqrtm
:stdm
:varm So yes, it looks like you've got all of them ( |
It is inevitable that this will lead to bugs that will be hard to track down, and that pretty much every "Intro to Julia"-tutorial will have a warning about this behavior. The question is if it is worth it to avoid having to do a type check in functions that want to do |
The |
While In fact, I actually think this will help some people out. This case on StackOverflow comes from a GSoC pre-application. This was late into the application process, and the issues that came up were because mathematics shorthand in papers tends to use notation that doesn't reference the matrices. At this point, the problem was not recognizing So |
And in actual code that is not meant to be a library that takes any array as input, when a function takes an square matrix as input in one case, it is highly likely that it'll always take square matrix (I don't think I've ever dealt with a matrix that's accidentally square) so it basically means that using square matrix will be really error prone. It is necessarily true that square matrices are used more often than There will always be people that don't read documents and misunderstand what a function does. Trying to match mathematical notation is not always (and usually not) a good idea and has already cause too much problem. If anything, raising a clear error and let the people choose will certainly help more people than picking one or the other as default.
Given the low number of complaint and (unfortunate but real) tradition of element-wise operation by default, the number of people that expect matrix op is certainly the minority, especially among the people doing numerical computation.
It will also mean that simple functions are unusable and intentionally confusing for people without reading all the docs.
I take this to mean that I have not learnt how to debug and correct code since I've certainly not learnt how to find this kind of errors without an error message. |
There are two somewhat ambiguous choices for what |
We don't force explicit disambiguation via |
It's true that it would cause confusion to Matlab / python people, but it does create a sense of mathematical consistency that makes it easy to remember ("ah yes, unlike those languages, julia actually does the right thing"). Also, what's the point of depreciating exp(arrays) if not to free up the notation for linear algebra? |
To be consistent about not having manually defined vectorized functions.
I don't think having
👍! |
Why not? Isn't that exactly what exp should mean? Either we have 3 spellings |
I'm not sure what you mean with should. |
I'm with Tony here. In Numpy, Fortran, and R, |
It is not easier to explain and the easiness to explain and understand is independent with how easy it is to make error. That's exactly why I was holding back my comment until I count the number of times I would have wasted a lot of time on this in the past few months. |
Tony said it well, as did Andreas. IMO, Julian's should really no longer expect functions to be automatically broadcasted to elements. I am one of those people that had bugs in MATLAB, and even in Julia, because I expected @KristofferC It seems obvious to me that @StefanKarpinski I view this in a similar light to the proposals around modifying |
As far as I see @yuyichao's point of view is that this introduces a potential to create a logical error in your code with no errors or warnings, by writing code that is almost indistinguishable from the correct version (i.e. I'd begin by refuting that Then there's the argument of consistency which enables generic programming. I think @eveydee had a great idea for an implementation of I also feel more generally that we should give |
No because those will NEVER produce the correct result.
This is unrelated to whether they "look similar"
That's exactly the opposite what I experience, especially when the code has a mix of scalar and element-wise operations.
Both
While that can be nice, it is unrelated to what we need to call the function. Also, any example of why such a def is useful? (In almost all cases I can think of, that is NOT how you want to define it).
I've already mentioned this that the difference is that these are operators so it's hard to find alternatives that's not way more complicated to write. |
Is there a compromise to be had here? Where I.e. dropping from Base: expm(x::Number) = exp(x) and adding:
Later While I'm at it, why is this less accurate? I guess not to important, at least that case..:
|
For
That's just a printing issue. |
If you define I think another problem with having both But I think there is the deeper issue that And I just realised this, but in numpy, |
I was just asking for a case where this is actually useful. Also, as I said, it's unrelated to what we call it.
Because such mathmatical consistency is not the top priority for a programming language. |
Just chiming in to say that I'm a primarily python/numpy user and part time Julia convert, and it seems obvious to me that |
Triage was unanimously in favor |
I like this change but,
? |
We have a weekly triage call at 12:15 Eastern on Thursdays on Google Hangouts. The link is in the #triage channel on the JuliaLang Slack. Anyone who wants to can join – although it's not exactly fun. Everyone on the call – which was about a dozen core contributors or so – was in favor. That seemed like enough to make the decision. |
Thanks all! :) |
So today I was part of a MATLAB workshop with relatively new programmers. It's interesting how much we cared about this, but how little they cared. I for one was humbled by how little importance some of this stuff really is (though it does matter, I think?!). Cheers all and I hope that this makes us all humbled by the fact that, in the end, it really doesn't matter all too much. |
Exactly. No one cares what to use when you write the code. That's why I'm talking about how much effort you'll need to debug the code!!! |
@@ -92,7 +92,6 @@ Base.repmat | |||
Base.kron | |||
Base.SparseArrays.blkdiag | |||
Base.LinAlg.linreg | |||
Base.LinAlg.expm |
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 guess this should just have been changed to Base.LinAlg.exp(::StridedMatrix)
rather than deleted?
Looks like we missed |
|
I realized that the |
They're not arbitrary at all: they're the most common bases to work in and they're in libm. They can also both be computed more accurately and efficiently than the generic |
I know they're in libm, but C doesn't have an exponentiation operator, and there is a much closer relation between API and implementation in C, so them having distinct implementations for performance is a good argument for why they should be separate functions. Esp with constant propagation that argument is much weaker here, and personally I'd prefer the uniformity of just writing |
Across the entire package ecosystem, I count no use of |
Now that vectorized
exp
is out of the way, this pull request deprecates the spellingexpm
for matrix exponentiation in favor ofexp
. Analogous pull requests forlogm
andsqrtm
to follow. (Do other*m
functions exist?) Ref. #19598 and #8450. Best!