Fix #126 (Add parens in util > print where necessary)

[sqrtsanta]

As @kevinbarabash proposed in #126, add parenthesis around any + (as well as -) operation if it isn't the root node. It also fixes #120 issue as well.

print({ "/" [ { "+" [ "x", "2" ] } "2" }) => "(x + 2) / 2"
print({ "+" [ { "+" [ "x", "x" ] } { "+" [ "2", "2" ] } ] }) => "(x + x) + (2 + 2)"

[evykassirer]

update: I'm heading home for reading week tomorrow and am not totally sure how much time I have, but I really wanna check this out so we can finish up this change :) I'll hopefully work on it in the car but I'll keep you updated

[sqrtsanta]

@evykassirer Ok. Btw it is actually hard to test this case, since nor removeUnnecessaryParens, nor flattenOperands does not remove parenthesis it this case - (2 + 3)x. It is removed between substeps in collectAndCombineSearch. Will work on this

[evykassirer]

alright I have investigated the broken tests and am here to report how I fixed it 🎉

step 1:

Cloning does help, because the flatten function changes the structure of the node, and sometimes it needs to stay the way it was for the next step to work. So some of the tests were fixed with this:

step 2:

The equation tests were broken for a totally unrelated reason - it was never turning the node into a string. It hadn't been caught yet, until I added that clone in, and then it was like "hey you can't clone a string" so I fixed that too

step 3: update the remaining tests that are failing.

one of them was related to flattening and getting rid of parens:

the rest are all because flattening makes some multiplication implicit and others not implicit and so it changes things around... so just update the tests like this to be passing again:

you good to add these changes into your diff? let me know if you have any questions :)

[sqrtsanta]

Ok, I've updated print.js according to comments & fixed the tests. Everything is great.

The last thing is to add new tests for this fix.

As I wrote before, the parenthesis were removed only between substeps in collectAndCombineSearch. Precisely, newNode after GROUP_COEFFICIENTS & oldNode before SIMPLIFY_ARITHMETIC, due to evaluateConstantSum.

If you pass node with parenthesis to evaluateConstantSum, it will return status with oldNode w/o parenthesis.

TestUtil.testSubsteps is checking newNodes only. Should I change TestUtil, so that it will check oldNodes as well?

[evykassirer]

ah yes, this is from the other bug that was found right?

I think testing old nodes would be great, yeah :) if it's straightforward to add, that'd help a lot. right now we're not testing them at all 😕

[sqrtsanta]

I added tests for old nodes, as usual some tests are failing.

before change: 2x + 4x + y
change: COLLECT_LIKE_TERMS
after change: (2x + 4x) + y
before change: 2x + 4x + y
change: ADD_POLYNOMIAL_TERMS
after change: 6x + y

The parenthesis are removed in evaluateConstantSum & combineLikeTerms, because of the expression like node = node.content;. Subsequent statuses are based on this mutated node, so status.oldNode is always w/o parenthesis. In order to fix this I need to make several changes like that:

  // STEP 2A: evaluate arithmetic IF there's > 1 constant
  // (which is the case if it's a list surrounded by parenthesis)
  if (Node.Type.isParenthesis(constants)) {
    const constantList = constants.content;
    const evaluateStatus = arithmeticSearch(constantList);
    evaluateStatus.oldNode = constants; // <- assign oldNode manually
    status = Node.Status.childChanged(newNode, evaluateStatus, 0);
    substeps.push(status);
    newNode = Node.Status.resetChangeGroups(status.newNode);
  }

It seems bad & dirty, is there a cleaner way to do this?

Also, if this is beyond the scope of this PR, I can just provide tests for print, but then I have to manually remove parenthesis before printing, like that.

function testPrint(exprStr, outputStr, fn) {
  let input = math.parse(exprStr);
  if (fn) {
    fn(input);
  }
  TestUtil.testFunctionOutput(print, input, outputStr);
}

describe('print asciimath', function () {
// ...
});

describe('print with parenthesis', function () {
  const tests = [
    ['(2 + 3)x', '(2 + 3) * x'],
    ['(7 - 4)^x', '(7 - 4)^x'],
    ['(9 + 2)/x', '(9 + 2) / x'],
  ];
  tests.forEach(t => {
    testPrint(
      t[0], t[1],
      (input) => { input.args[0] = input.args[0].content }); // <- like this
  });
})

[evykassirer]

If I'm understanding what's going on correctly (correct me if not) I think you can fix with clone (and that's actually why we started cloning in the first place! but we probably missed a few places)

"Subsequent statuses are based on this mutated node" - if something's being mutated, it should clone before mutating, so the original node stays the same

childChanged is used in the case where the content of a parenthesis node is changed, so it should add the parens back around both the old node and new node... hm

I'm a bit tired today so I don't have the brainpower to figure out where the clone would happen, but take a crack and it and I can take a look later today or tomorrow if you need another pair of eyes on it :)

[evykassirer]

I think it also is reasonable to just put this in another PR, especially if that's easier (either is fine for me)

why do you have to remove the parens before printing? how will the test fail otherwise? (this seems unrelated to the other issue, right?)

[evykassirer]

hey btw do you want some mathsteps stickers as a thank you for your contributions? 😸

if you're interested, email mathsteps@socratic.org with your mailing address and I can send them over!

[sqrtsanta]

why do you have to remove the parens before printing? how will the test fail otherwise? (this seems unrelated to the other issue, right?)

They will not fail. The point of this PR is to print ({ "/" [{ "+" ["x", "2"] }, "2"] }) => "(x + 2) / 2". It is just hard to reproduce this node, since nor removeUnnecessaryParens, nor flattenOperands does not remove parens, they are removed between substeps in evaluateConstantSum.

[evykassirer]

Ohhh okay I'd say just create nodes with the mathjs constructors then, like in the flatten tests. Do you think that would work?

[sqrtsanta]

Ohh, how did I miss that? Yeah, totally, will add ASAP

[evykassirer]

everything looks good! just fix the linter issues

https://travis-ci.org/socraticorg/mathsteps/builds/203692251

(you can run npm run lint to see them locally)

[sqrtsanta]

DONE [x] Thank you a lot for helping me with this PR.

[evykassirer]

perfect! thanks so much for taking this on!

are you up for taking on the other change we were discussing in another PR?

also don't forget to email us if you want stickers ^__^

🎉 🎉 🎉

[sqrtsanta]

Yeah, I planned to check it carefully before creating an issue.

childChanged is used in the case where the content of a parenthesis node is changed, so it should add the parens back around both the old node and new node... hm

I've sent an email stickers ^^

[evykassirer]

hi @Flyr1Q are you still interested in working on the followup PR to this?

[sqrtsanta]

sorry @evykassirer, don't think I'll be able to work on this in the near future :(

[evykassirer]

no problem :) thanks for replying so quickly