-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove element wise operations of exp and gamma to prevent confusion #2440
Comments
Thanks for bringing this up. I wasn't aware of that, but it makes sense to remove the matrix support from both This would be a breaking change, so we will need to publish it as a new major version, which is fine (that is what the version numbering is for). |
OK. Is there any particular protocol then with respect to a PR that makes this change, and/or any particular timing? Other interface-breaking items that are queued up that need to get in around the same time? Just let me know and I can make a PR for this; and also the PR in progress for |
There is not a specific protocol for breaking changes. If there are multiple breaking changes it's of course nice to bundle them in one breaking release. So in this case we could bundle it with the breaking change in #2370 for example. |
good point! |
OK, I will put in a PR right away to remove the elementwise operation from gamma and exp so that you have flexibility for when to merge and generate a new major version. (If it sits for a bit because you want to wait on a major version bump, I totally understand.) |
When I looked up the matrix exponential, it referred to the matrix logarithm and hyperbolic sine, which in turn referred to the matrix sine, etc. Basically any function defined by a simple power series has an analogue for square matrices that provides the usual definition of that function on square matrices. So the PR will eliminate all of them. If you want to draw the line somewhere else, just let me know and I will revise. Incidentally, the current typing of say |
Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves josdejong#2440.
Yes the correct TypeScript definition is |
Yes, I guessed that's how you would respond, so in fact this change in typing is already in #2465. |
👍 |
Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves josdejong#2440.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
* chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement.
…ter (#2559) * chore: Prevent confusion with standard matrix functions. (#2465) * chore: Prevent consfusion with standard matrix functions. Prior to this commit, many functions operated elementwise on matrices even though in standard mathematical usage they have a different meaning on square matrices. Since the elementwise operation is easily recoverable using `math.map`, this commit removes the elementwise operation on arrays and matrices from these functions. Affected functions include all trigonometric functions, exp, log, gamma, square, sqrt, cube, and cbrt. Resolves #2440. * chore(typescript): Revise usages in light of changes sqrt() is now correctly typed as `number | Complex` and so must be explicitly cast to number when called on a positive and used where a Complex is disallowed; sqrt() no longer applies to matrices at all. * feat: Provide better error messages for v10 -> v11 transition Uses new `typed.onMismatch` handler so that matrix calls that used to work will suggest a replacement. * fix: prevent chain from matching rest parameter with stored value Since the revised code needs the isTypedFunction predicate, switch to using the typed-function implementation for that throughout mathjs, rather than rolling our own here. Also adds a test that chain() no longer allows this kind of usage. Removes the two type declarations in types/index.d.ts that were allowing this sort of "split rest" call and added tests that such calls are forbidden. Adds to the chaining documentation page that such "split" calls are not allowed. * chore: Refresh this PR to reflect underlying changes Also addresses the review request with a detailed comment on the correctness of a new code section. Note that it reverts some changes to the TypeScript signatures of the matrix functions ones() and zeros() -- they do not actually have a typed-function signature of two numbers and an optional format specifically for two dimensions. What they have is a single rest parameter, from which the format is extracted if present. Hence, due to the ban on breaking rest parameters, it is not valid to call math.chain(3).zeros(2) to make a 3-by-2 matrix of zeros, which seems like a perfectly valid ban as the division of the dimensions is very confusing; this should be written as math.chain([3,2]).zeros(). The TypeScript signatures are fixed accordingly, along with the edge case of no arguments to ones() and zeros() at all, which does work to produce the "empty matrix".
Hi, first of all I’m constantly impressed with the capabilities of this library and fast pace of development. I would like to discuss the topic of an alternative to this decision.
The benefit is that it allows for simpler evaluations. There are some examples of the workings of these functions in matlab and numpy using both scalars and N dimensional arrays. As shown in the previous examples matlab and numpy can use exp and gamma in both ways. I have not used the function gamma, but the array like capabilities of exp seems as important as the array capabilities of sin or cos |
Thanks for the references and the feedback. I don't see the matrix exponential in numpy, but I do see that matlab has |
Thanks for reviewing this and for the research on Even though numpy doesn't have I recently encountered one case where # If the input is a scalar use exp(input), if it's an array use map(input, exp)
Exp(input) = equalText(typeOf(input), 'number') ? exp(input) : map(input, exp) As you stated on other projects I see that the same change has been happening for these functions
I did some quick tests on these functions with numpy/scipy and Matlab and it's the same difference for all. It's expected to work for scalars and element wise for matrices and arrays. I understand this is to avoid some confusion with matrix operations, nonetheless I prefer the element wise option available on numpy/scipy and Matlab as it's very convenient to avoid some loops or maps. I think I was writing my opinion too categorically, please note that I respect your decision and I will work around these functions that do not support matrices and arrays anymore. Maybe a rephrasing of this statement is in order for the newer version
|
Sorry for the confusion on I think there is no need for introducing a new tuple kind of data structure to hold function arguments. I would like to avoid that because, like you explain, it brings a lot of complication. We can handle arguments on the right hand side of the Implementation could roughly look something like this:
|
OK, wow, I was not thinking about this clearly, sorry. If a proposal along these lines is adopted, then we want the meaning of If so, then the fact that currently ranges accept optionally a third element as in If you don't like that for binary vectorization, then yes, In any case, since all of these So in other words, the |
Oh and the questions in my point (4) above are all still valid; I truly was asking about the existing square function, and also about any other existing vectorizing functions. |
I think you're right and we do not need an SyntaxWe could indeed consider interpreting Mixing the original numeric range with the new element-wise function operator can introduce quite some confusion and weird cases. Maybe we should simply not allow
It feels a bit weird indeed to extend PhilosophyAbout your point (4) on
I'm not sure if there are still functions in a "gray" area in this regard, but I think |
Yes, I think we are homing in on the details here. But I think there are still some parsing issues:
This is currently a syntax error, and if we are not going to use this for applying a ternary function
The first of these has to parse to a RangeNode with three children, in case foo, bar, and baz all mean numbers. So if the second form also parses to a RangeNode with three children, and the semantics of such a node is given by the
In option B above, this will become very problematic, particularly in the case So to sum up all of these points, I see four ways to go at the moment, in no particular order. (i) Leave things be: mix of non-conflicting operators auto-vectorizing, and needing explicit (ii) Extend (iii) Extend (iv) Use a different operator symbol than |
Edge case, ranges as arguments of a functionPlease consider the case for ranges as inputs in element wise functions with multiple arguments. add(1:1:3, 3:-1:1) # [4, 4, 4]
subtract(1:1:3, 3:-1:1) # [-2, 0, 2]
dotMultiply(1:1:3, 3:-1:1) # [3, 4, 3]
dotDivide(1:1:3, 3:-1:1) # [0.33333333333333, 1, 3]
dotPow(1:1:3, 3:-1:1) # [1, 4, 3] Reference from Wolfram LanguageI fount in the Wolfram Language docs something that might be useful regarding the discussion for mappings and element wise functions. I found it interesting as it's Mathematical rather than numerical (like Matlab)
|
Right, I think all of these proposals will be compatible with a numeric range as an input, e.g. And thanks for the Wolfram reference; it means that if we choose to go with an alternate operator symbol, we could choose |
Good analysis Glen! I hadn't realized these side effects of weird cases like I'm starting to be convinced that we should not reuse Let's see if we can settle on an alternative symbol for the map operator. Thanks David for looking up the syntax that Wolfram uses!
So far I like both |
Purely in the spirit of brainstorming: the sort of implementation that's being proposed here makes this not really a new operator (infix notation for a function applied to its left and right operands) but a variant function application notation, that vectorizes rather than applying to entire collections as single entities. So conceptually this suggests to me that the difference should be some change to the ordinary function-application-notation
Upon initial reflection, I think I like option (5) best, and I think any of the three symbols |
And in fact I will say I like (5) with |
That is an interesting idea! So instead of putting a mark on the function, we could put it on the actual argument that we want to iterate over. Conceptually it makes sense. And it gives some interesting new flexibility dealing with element-wise operations. I think though that this concept is a bit more abstract to understand than the element-wise function call like I agree with you that I'll give it a bit more thought 🤔 |
Weellll, since there is currently no definition of multiplication of a function and a vector, mathjs conceptually could define |
Ha, that is true. It feels quite confusing to me though to extend |
Well, to my eye, "classic" range and elementwise application were just two distinct operations we were considering gluing together for notational convenience. Here at least, if someone said "Hmmm, we have no definition for the product of a function and a vector, what should that do?" there's not much else that comes to mind other than applying the function to each element of the vector; and we already have some precedent for "special" meanings of * for vectors because of the decision to use |
In any case, |
yeah it's not conflicting in any way. I prefer to not go that direction though. Latest thoughts from my side: I think I prefer I do like your proposal of
|
I was doing some tests related to numerical integration of ordinary differential equations that could be used in problems like Rocket Trajectory Optimization and it seems promising. I think it has the potential to be more versatile than what can be found in numpy or Matlab as here we can also include units and that would make representing physical systems clearer. The reason I bring this to your attention is that even though I respect the decision to remove the element wise operations to avoid confusion and be more mathematically sound, I think it might affect numerical computing a bit. These new ways of mapping functions in the parser are very promising and I was wondering if it's possible to cover both cases? I mean for most functions to accept array likes and also have this versatile ways to map in the parser. Since the element wise operations were removed I've encountered a few minor issues:
|
Do you mean to keep support for matrices in say function |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
On the amended question, my opinion is that since there is no matrix-analytic meaning of the "ceiling of a matrix" other than the element-wise ceiling, I don't see any harm in continuing to allow |
In particular I mean for functions like I encountered a case while implementing a numerical ODE integrator and the function to integrate includes one of those singleton functions. It's common for these methods to use multiple initial values. y_prime(t, y) = sin(y) + cos(y+1)
initial_state = [1, 1.1]
h = 1
t = 0
next_state = initial_state + h * y_prime(t, initial_state) This throws an error due to the use of a vector inside A workaround is that the user of the numerical ODE integrator defines y_prime function in a map if the function includes any singleton function. y_prime(t, y) = map(y, sin) + map(y+1, cos) Or maybe it's more legible for it to be y_prime(t, y) = map(y, _(y1)=sin(y1)+cos(y1+1)) Or manually mapping y_prime(t, y) = [sin(y[1])+cos(y[1]+1), sin(y[2])+cos(y[2]+1)] With the new map nomenclature it might be more legible, but I don't know outside the parser how it would work. y_prime(t, y) = sin@(y) + cos@(y+1) This is an example of a possible inconvenience that I encountered during testing in comparison to other numerical ODE integrators. |
Right, there's clearly a need for a function that will be say exp of a number and elementwise exp of a vector. The point of this thread is to settle on what the notation for that should be. I think Jos has settled on |
So far I like |
I was reading a similar discussion in the Julia community.
After reading that I’m detracting from my request to allow both methods. It seems to work well for them and cover more cases. If anything I would only question if in the meantime of this element wise operator is implemented you might consider returning back the old matlab style behavior. Now that |
Thanks for these references. I think they do reinforce where this discussion seemed to end up, which is for mathjs not to be concerned with making raw functions work on both scalars and matrices, but instead to provide convenient notation for vectorizing any function. I just am not sure if that notation ever quite settled... |
Thanks for the review Glen, Today I was reading in detail the blogpost in which they announced the change they made which is very inline with this discussion. https://julialang.org/blog/2017/01/moredots/ In particular I found interesting they use some of the characters mentioned here.
And also their description of broadcast and how it would apply for functions with multiple arguments. Something interesting is that for Julia the f(a,b) = a+b
f.([1,2],[3,4]') Which in Matlab, Numpy and |
Really interesting to see this very same discussion for Julia. We indeed still have to make a decision here, my personal preference so far is a notation like |
There is a standard meaning for the Gamma function applied to a square matrix (see https://arxiv.org/abs/1806.10554), which is not the Gamma function applied componentwise, like the current behavior of
gamma(M)
in mathjs. Also, the componentwise application already has another very short expression, namelymap(M, gamma)
.To avoid potential confusion, should the current matrix interpretation of
gamma(M)
as componentwise application be eliminated, requiring the use ofmap(M, gamma)
? That way, no one would accidentally callgamma
on a square matrix thinking they were getting the usual matrix gamma function. (And if anyone ever chose to implement the usual matrix gamma, it could either be overloaded onto the gamma symbol, or calledgammam
by analogy withexpm
.)In fact, the analogy with
exp
is very strong. The usual mathematical meaning of "exp" of a (square) matrix is not componentwise exponentiation, but rather the matrix exponential. Clearly a specific decision was made to call that operationexpm
in mathjs, and I am not suggesting that decision be revisited. But I would suggest that the componentwise meaning ofexp(M)
be disabled and the documentation ofexp
be revised to point toexpm
for the matrix exponential andmap(M, exp)
for componentwise exponentiation. Again, the motivation here is to avoid the potential confusion of someone callingexp(M)
thinking they were gettingexpm(M)
.Failing that, I'd suggest amplifying the documentation of exp to make it more prominent that
exp(M)
is not the matrix exponential.In any case, perhaps it would be good to make a decision at least on whether
gamma
should continue to apply componentwise to matrices before acting on PR #2416 (which is the proximate cause of my looking into the semantics ofgamma
and raising this issue).The text was updated successfully, but these errors were encountered: