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

math.derivative changed result between 3.14 and 3.16.1 #1013

Closed
javazen opened this Issue Jan 12, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@javazen

javazen commented Jan 12, 2018

First, thanks to Jos and everyone who has made this a great math library! I have unit tests for the parts of mathjs I use, and when I upgraded from 3.14.2 to 3.16.1 one of them that uses math.derivative() started failing.

For the expression "1 - (-B)" math.3.14.2 gives a derivative node which is a ConstantNode, value=1, which is correct.

In 3.16.1 and later, the same expression gives a derivative node which is a unaryMinus OperatorNode. Its only arg is a ConstantNode, value=1. So the derivative is -1, which is incorrect.

When I debugged thru the derivative call, it looks like what changed is the simplify rule:
//{ l: 'n--n1', r:'n+n1' }, // simplifyCore

Or am I missing something? Thanks, Sam

@joelhoover

This comment has been minimized.

Contributor

joelhoover commented Jan 12, 2018

Can confirm that this is an issue in the most current version (3.19.0).

So, this is actually an issue in simplify, because if we take the derivative with options.simplify set to false, we get 0 - -1, which is correct. However, we run into issues if we run simplify on such expressions:

simplify("0 - -1") // "-1"
simplify("0 - -x") // "-x"

Diving into an invocation of simplify on "0 - -x", after simplifyCore runs we have -(-x), which is correct. However, after the next rule ({ l: 'log(e)', r:'1' }) is applied, simplify now has -x. This transformation from -(-x) to -x actually happens in util.flatten, which flattens all associative nodes into a multibranching operator. Finally, the reason for this error is that internal to flatten, the unary minus operator is considered associative because it only has one argument:

if (!node.args || node.args.length <=1) {

Thus, flatten (or more precisely, the allChildren helper) combines all iterated instances of the unary minus in -(-x) into one instance, thus negating the sign in many case.

To fix this, I recommend that we remove the node.args.length <= 1 check in isAssociative (and in isCommutative), so that unary minus is no longer considered an associative operator. @josdejong Please let me know what your thoughts are on this fix.

While debugging this case, I found a related but more complicated (and more severe case): simplify("0 - (x - y)") gives -x. Now the root cause here is the same, but more insidious: flatten("-(x-y)") gives x - y, because even though the outer - is unary and inner one binary, allChildren only compares the op when flattening the OperatorNodes, so they will be flattened. Even worse, allChildren just copies the fn property from the outer node, so the x - y that is returned is actually an OperatorNode with fn = 'unaryMinus'! Later, this causes the later argument y to be dropped, since the - should be unary. The root of this entire issue is the same as the 0 - -x one (and so the above fix of changing isAssociative will fix this issue as well), but I though this whole process of getting -x from 0 - (x - y) was rather interesting.

@firepick1

This comment has been minimized.

Collaborator

firepick1 commented Jan 13, 2018

Joel, thanks for tracing this through to code with which I am unfamiliar. I can help with simplifyCore should the need arise.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 13, 2018

Thanks a lot @joelhoover , your research saves me quite a headache ;) .

I think you're right: we cannot distinguish subtract and unary minus by their operator, we have to rely on their name instead in isAssociative, and the check on args<=1 was in the way there. I still want to make a small refactoring to isCommutative, probably add a test whether the node is an OperatorNode before checking the function (else everything is suddenly considered not commutative...)

I've applied your fix here: 8724ae0, will do a release today or tomorrow.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 13, 2018

@joelhoover I've done a small refactoring in isAssociative and isCommutative to first have them check explicitely whether dealing with an OperatorNode instead of indirectly via a number of optional args, see commit 18f703b. Does that make sense?

@josdejong josdejong closed this in 8724ae0 Jan 14, 2018

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 14, 2018

These issues should be fixed now in v3.20.0, thanks again Joel 👍

@joelhoover

This comment has been minimized.

Contributor

joelhoover commented Jan 14, 2018

Always a pleasure to help. 😄

Just a quick question, is there a reason why isCommutative returns true for non-OperatorNodes? It seems like it should return false like isAssociative does.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jan 15, 2018

Well, honestly, I have left that in place because else a couple of unit tests break, I haven't looked into it deeper (I haven't written the code). I suppose isCommutative should be true for non-operators because it's not applicable in that case. Or maybe the list with commutative operators should be extended or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment