Negated fraction multiplied implicitly not stringified correctly

[thatcomputerguy0101]

When using math.parse to parse an expression, "1 / 2 a" turns into (1 / 2) * a, as expected, but "-1 / 2 a" turns into ((-1) / (2 * a)), not ((-1) / 2) * a or ( -(1 / 2)) * a as expected. This may not seem like too much of a problem, but it does mess up when an equation is simplified correctly, stringified with parenthesis set to auto, and then reparsed, such as in the below.

math.parse(math.simplify("-(1 / 2) * a").toString({parenthesis: "auto"})).toString({parenthesis: "all"}); // returns "(-1) / (2 a)", which is not equivilent to "-(1 / 2) * a" which was started with

For some reason, math.parse will return the expression with the parentheses inserted, while math.simplify won't. So either math.simplify needs better parenthesis insertion, or math.parse needs to parse it's equations slightly differently.

[josdejong]

The parser parses a unary minus as an operator, it does not negate when followed by a number. The confusion here is probably that you're using implicit multiplication, which has certain heuristic rules and is always tricky to use!

In your code example you have a typo in parenthsis, that should be parenthesis. I cannot reproduce the outcome that you get though.

It may be an interesting improvement in the parser to parse a unary minus followed by a number in a constant with the negated value. Or at least, have simplify resolve this, as discussed in #1406.

[thatcomputerguy0101]

It appears as if it was some change that I made to the simplify ruleset that caused the issue with the script that I presented. I should have done more testing before I reported it as an issue. Although, I still find it odd how the unary minus at one point changes the precedence of the implicit multiplication further along int the expression.

[josdejong]

Ok!

Although, I still find it odd how the unary minus at one point changes the precedence of the implicit multiplication further along int the expression.

That should not be the case, and so far I haven't been able to reproduce such behavior. If you have an example please let me know.

[thatcomputerguy0101]

math.parse("1 / 2 a").toString({parenthesis: "all"}) // => "(1 / 2) a"
math.parse("- 1 / 2 a").toString({parenthesis: "all"}) // => "(-1) / (2 a)"

In the first example, the a is parsed as multiplied by 1/2, while in the second example, it is part of the divisor.

RunKit example at https://runkit.com/embed/4zht1kqm9k1i

[josdejong]

Ah, yes. That is basically intended behavior, as in, there are a number of rules to determine when to give implicit multiplication higher precedence then division, and this is an example where it doesn't look logical. There are many more examples of this, but the basic culprit is that using implicit multiplication is tricky because it's very ambiguous.

math.parse("1 / 2 a").toString({parenthesis: "all"}) // => "(1 / 2) a"
math.parse("- 1 / 2 a").toString({parenthesis: "all"}) // => "(-1) / (2 a)"
math.parse("5 - 1 / 2 a").toString({parenthesis: "all"}) // => well, can you guess?...

See #792 for more details

[thatcomputerguy0101]

I finally found a way to share my problem with this. The mathjs object represented by the following JSON, when converted to a string with parenthesis set to auto and then reparsed, comes up with a different order of operations.

{"mathjs":"OperatorNode","op":"*","fn":"multiply","args":[{"mathjs":"OperatorNode","op":"/","fn":"divide","args":[{"mathjs":"OperatorNode","op":"-","fn":"unaryMinus","args":[{"mathjs":"ConstantNode","value":1}],"implicit":false},{"mathjs":"ConstantNode","value":2}],"implicit":false},{"mathjs":"SymbolNode","name":"a"}],"implicit":true}

Reprsented object with parenthesis set to all: ((-1) / 2) a
Reparsed object with parenthesis set to all: (-1) / (2 a)

In case I didn't explain my issue well enough, here's another RunKit example: https://runkit.com/embed/4xsmd0abvhox

[josdejong]

ahh, thanks for sharing! That helps a lot.

here some extra output of your example so we can understand what happens:

start = JSON.parse('{"mathjs":"OperatorNode","op":"*","fn":"multiply","args":[{"mathjs":"OperatorNode","op":"/","fn":"divide","args":[{"mathjs":"OperatorNode","op":"-","fn":"unaryMinus","args":[{"mathjs":"ConstantNode","value":1}],"implicit":false},{"mathjs":"ConstantNode","value":2}],"implicit":false},{"mathjs":"SymbolNode","name":"a"}],"implicit":true}', math.json.reviver)

console.log(start.toString({parenthesis: "all"}))                // ((-1) / 2) a
console.log(start.toString({parenthesis: "auto"}))               // -1 / 2 a
console.log(math.parse(start.toString({parenthesis: "auto"}))
  .toString({parenthesis: "all"}))                               // (-1) / (2 a)

What happens here is that when stringifying the implicit multiplication, it should put parenthesis around it's left and right hand side if that turns out to be an OperatorNode.

This cannot happen when parsing an expression I think, since you have to use parenthesis in order to create such a node tree. It can happen though when you transform a node tree so it would be a good idea to improve OperatorNode such that it adds parenthesis when needed. I think the code for that is already there, but probably does not reckon with implicit multiplication and concludes that parenthesis are not needed because division and multiplication have the same precedence.

Anyone interested in fixing this?

[shenzhuxi]

This is because parseRule2() in parse.js only deals with [number] / [number] [symbol] like 1/2x.
-1/2x: unaryMinus will not be proceesed.
x/2x: SYMBOL will not be proceesed either.

I don't understand why 2 x / 5 y = (2 x) / (5 y) in #792. I thought AsciiMath should be parsed as http://asciimath.org/.

[josdejong]

This is because parseRule2() in parse.js only deals with [number] / [number] [symbol] like 1/2x.
-1/2x: unaryMinus will not be proceesed.
x/2x: SYMBOL will not be proceesed either.

Yes you're right @shenzhuxi

I don't understand why 2 x / 5 y = (2 x) / (5 y) in #792. I thought AsciiMath should be parsed as http://asciimath.org/.

I'm not sure what you mean, we're not talking about AsciiMath here but about the syntax of mathjs.

[shenzhuxi]

I'm not sure what you mean, we're not talking about AsciiMath here but about the syntax of mathjs.

if "1 / 2 a" = "(1 / 2) * a", it's consistent for "x / 2 a" = "(x / 2) * a" and "2 x / 5 y " = "(2 * x / 5) * y" but not for "2 x / 5 y = (2 x) / (5 y)".

If "2 x / 5 y = (2 x) / (5 y)", "1 / 2 a" should be "(1) / (2 * a)".

The syntax of mathjs doesn't have to be the same as AsciiMath. At least AsciiMath syntax is consistent at this point.

[josdejong]

The precedence of implicit multiplications is indeed not very simple in mathjs, there is good reasons for this (and unfortunately no silver bullet). There is an in-depth discussion on this in #1793 #792.

[shenzhuxi]

Are you sure it's about underscore? I guess you mean #1502.

[josdejong]

Are you sure it's about underscore? I guess you mean #1502.

😂 oh sorry, I mean #792

[gwhitney]

I can confirm this issue still exists exactly as described in mathjs 10.1.0. I would be happy to (try to) produce a PR. Before I get started on that, I just wanted to confirm that the desired resolution is to leave the parsing of -1 / 2 a alone, but change the result of .toString() called on the Node created as start in the code snippet above to something like (-1 / 2) a that will parse to a mathematically equivalent expression as start?

[josdejong]

@gwhitney thanks for picking this up. It's quite some time ago 😅 but indeed like I explained in #1431 (comment), it is about reckoning implicit multiplication correctly in .toString(), and not related to parsing.

[gwhitney]

OK, I have looked into this a little bit. I have two questions:
(1) Is there currently unit testing of .toString()? I couldn't find any direct testing thereof, just implicit testing through things like simplify testing that compares the .toString() outputs of various trees. But since .toString() itself clearly has some tricky cases, it seems to me that it would be best to have some explicit tests for it. Therefore, in the PR that addresses this, I would propose to add test/unit-tests/expression/stringify.js. Does that sound good?

(2) There is already one sort of special case in calculateNecessaryParentheses for products, right at the end:

    // handles an edge case of 'auto' parentheses with implicit multiplication of ConstantNode
    // In that case print parentheses for ParenthesisNodes even though they normally wouldn't be
    // printed.
    if ((args.length >= 2) && (root.getIdentifier() === 'OperatorNode:multiply') && root.implicit && (parenthesis === 'auto') && (implicit === 'hide')) {
      result = args.map(function (arg, index) {
        const isParenthesisNode = (arg.getIdentifier() === 'ParenthesisNode')
        if (result[index] || isParenthesisNode) { // put in parenthesis?
          return true
        }

        return false
      })
    }

Do you have a specific example of an expression (Node) that this is intended to handle? I definitely feel like there should be at least one specific test for this special case. Thanks!

[josdejong]

(1) there are unit tests for toString() for any of the Node classes, you can search for "should stringify ..." test descriptions in the folder /test/unit-tests/expression/node (or search for .toString() occurrences in that folder). I do not have a strong opinion, so if you would like to introduce a test/unit-tests/expression/stringify.js that is fine with me. So far, for example OperatorNode has a lot of special cases with .toString(), and all the tests are simply in OperatorNode.test.js, that makes sense to me since the corresponding logic is in OperatorNode.

(2) I see this code is introduce in the following commit: b285739. It has clear corresponding unit tests, for example "should stringify implicit multiplications between ConstantNodes with parentheses". Does that have the examples that you're looking for?

[gwhitney]

Thanks so much for the guidance; as you can see, I was able to work up a PR. I did also want to mention the relationship of this issue/PR #2403 to issue #2370, which suggested that the parsing of -1/2a should lead to (-1/2)a, which would make a change to .toString() unnecessary. Since openness to that change was expressed in #2370, it seems like it might be best once and for all to decide which way -1/2a ought to parse, and either close that issue as wontfix and take this PR and resolve this issue, or reject this PR and close this issue as wontfix (in which case I would be glad to try to work up a PR to address the parsing issue).

Note I am completely neutral about which way -1/2a would be parsed, I feel that's a close call and entirely up to the author; I just wanted to call attention to the seeming dichotomy between this and the other issue, so you can be sure to have in mind which way you want to go before merging #2403 (and can then probably close both issues; if there remains a chance you would entertain the change requested in #2370, it would not seem to make sense to merge the PR associated with this issue). So just trying to help keep the possibilities organized.

[josdejong]

Thanks @gwhitney for clearly pointing out that if we decide to change the behavior of parsing the unary minus (and unary plus) discussed in #2370, this will resolve this issue and make #2403 redundant. I hadn't realized that 😅 .

If I must decide, I prefer changing the behavior of -1/2a to be parsed as (-1)/2*a instead of the current behavior (-1)/(2*a), so favor #2370 over #2403. The reason is that I think that this results in more "consistent" behavior of the parser.

[gwhitney]

Although I just submitted PR #2460 implementing the revised rule for precedence of division over implicit multiplication as described in #2370, and in fact the exact case described here does now work correctly (and I added a test for it in #2460), I did not claim that PR resolves this issue because there are still other inconsistent cases. For example, if there is a negation in a denominator of an implicitly multiplied fraction, stringifying and parsing the result produces a non-equivalent expression. I.e., the following assertion fails:

    const coeff = new OperatorNode('/', 'divide', [
      new ConstantNode(1),
      new OperatorNode('-', 'unaryMinus', [new ConstantNode(2)])])
    const variable = new SymbolNode('a')
    const start = new OperatorNode('*', 'multiply', [coeff, variable], true)
    const startString = start.toString()
    const finish = math.parse(startString)
    /* FIXME: this test currently fails, related to #1431: */
    assert.strictEqual(
      finish.toString({ parenthesis: 'all' }),
      start.toString({ parenthesis: 'all' }))

So this issue should remain open, and a new PR similar but not identical to #2403 is needed to correct the parenthesization in cases of .toString() of an OperatorNode like the above example.

[josdejong]

Thanks @gwhitney , your solution looks very neat.

Good point that that the regular Node.toString() (without parenthesis: 'all') can now output a string that will not be parsed the same again. To solve that, I think we can extend .toString() to also check isRule2Node(...), and if so, add additional parentheses. Does that make sense?