Simplify: improve constant op. eval. in non-commutative contexts

[samueltlg]

Addresses the constant evaluation aspect as discussed in #2825

[gwhitney]

This is a very nice contribution! But the improved constant folding in foldOp can still be done in one pass, not two: as you go through the argument list, keep a (possibly empty) tree and a (possibly empty) operand. If the next argument is not a Node, try to fold it with the current operand, or if there is none, make it the current operand. Otherwise, coalesce the tree so far and the operand (reverted to a constant node), if any, and the new argument into the new tree (with the running operand empty). I think a single-pass algorithm like this would be more concise and easy to understand. Could you try to adjust your PR along such lines?

[samueltlg]

Hello and thank you @gwhitney, and thanks for the suggestion!

I did wonder whether it was possible to do with one pass, but decided against thinking it through under the assumption that it would be a bit inelegant or a bit of a hassle. But you are right, checking ahead should provide enough info for, as you say, knowing whether to collapse the current operand into the newly reduced tree (or create the tree/first node). I will make some changes later today and hope to hear what you think of the next iteration !

[samueltlg]

Hello @gwhitney,
I have made changes along the lines of what you have suggested - but it took me a lot longer than expected and also did not turn out so elegant. It was the simplest I could make it in my famished and tired state! But in any case was tricky to deal with all of the eventualities that came along with the introduction of an extra variable (operand). Not sure if it is readable much to you? Feel free and do make/suggest any changes.

[josdejong]

Thanks @samueltlg! This solution is less complicated than I had expected beforehand :)

It was the simplest I could make it in my famished and tired state! But in any case was tricky to deal with all of the eventualities that came along with the introduction of an extra variable (operand). Not sure if it is readable much to you? Feel free and do make/suggest any changes.

I understand what you mean: reading both approaches for the first time, I think the first is easier to understand because there are two cleanly separated processing steps. I'm not sure if we can simplify the second approach, there is a lot of things going on at the same time making it harder to understand. I will give it some thought. What do you think @gwhitney ?

[gwhitney]

Here's a version that I think makes it clearer what is going on; it's a little lengthy, but I think each step is very simple. I will also post a one-pass reduce-based version that is shorter but I think harder to understand. The main reason I really don't like the original two-pass PR is that it duplicates that tricky try-catch scheme for folding constants, so it is not DRY, and I find it very confusing why constants are being folded twice (in fact, the first pass does everything except assemble the binary tree, which is not at all obvious, so that two-pass code is also kind of deceptive).

  function foldOp (fn, args, makeNode, options) {
    let tree // the Node to return, unless everything folds into a constant
    let value = args.shift() // Running constant we are folding into, if any
    if (isNode(value)) {
      tree = value
      value = undefined
    }
    for (let item of args) {
      if (!isNode(item)) {
        // Fold new constant into the running value, if possible.
        if (value === undefined) {
          value = item
          continue
        }
        try {
          value = _eval(fn, [value, item], options)
          continue
        } catch (ignoreandcontinue) {
          // can't fold, make into a Node and fall through to that case:
          item = _toNode(item)
        }
      }
      // Incorporate everything so far into the tree; conceptually this is
      // makeNode([tree, _toNode(value), item]) except we want a
      // binary tree and we have to deal with tree or value being undefined:
      if (tree === undefined) {
        tree = _toNode(value)
        value = undefined // We used the value
      }
      if (value !== undefined) {
        tree = makeNode([tree, _toNode(value)])
        value = undefined
      }
      tree = makeNode([tree, item])
    }
    // All arguments processed. Return whichever of tree or value is defined,
    // or combine them if both are:
    if (tree === undefined) {
      return value
    }
    if (value === undefined) {
      return tree
    }
    return makeNode([tree, _toNode(value)])
  }

[gwhitney]

Also, as submitted, I think the original 2-pass version will fail on something like a + 0 + 7 + b where the first operand in an internal group that needs to be coalesced happens to be a "falsy" value, since the if (a && ...) part of the condition in the first pass will fail. That would need to be tested. And now I am also worried my version above will fail if one of the args happens to be the constant undefined -- I think it will not try to fold it with the next arg, but instead just replace it with it. So it might well fold undefined + 7 to just 7, which is not right -- that would have to be tested.

[gwhitney]

OK, here's the promised reduce-based version. It's a little mysterious, but not so much more so than the code before this PR with its slightly odd triple test of whether a and b are Nodes or not (which is gone in this version). It is significantly shorter than my "clearer" suggestion, and as far as I can tell does not suffer from any problems with falsy or undefined constants. It also only does the try-catch for folding constants in one place. So I'd say on balance it is my recommendation. I will leave it to the two of you to pick among the various options (or some evolution of any of them) but please be sure to test on 0 and '' and undefined inputs. Thanks!

function foldOp (fn, args, makeNode, options) {
    const first = args.shift()
    // In the following reduction, sofar always has one of the three following
    // forms: [NODE], [CONSTANT], or [NODE, CONSTANT]
    const reduction = args.reduce((sofar, next) => {
      if (!isNode(next)) {
        const last = sofar.pop()
        if (isNode(last)) {
          return [last, next]
        }
        // Two constants in a row, try to fold them into one
        try {
          sofar.push(_eval(fn, [last, next], options))
          return sofar
        } catch (ignoreandcontinue) {
          sofar.push(last)
          // fall through to Node case
        }
      }
      sofar.push(next)
      // Collapse everything so far into a single tree:
      return [sofar.reduce(
        (a, b) => makeNode([_ensureNode(a), _ensureNode(b)])
      )]
    }, [first])
    if (reduction.length === 1) {
      return reduction[0]
    }
    // Might end up with a tree and a constant at the end:
    return makeNode([reduction[0], _toNode(reduction[1])])
  }

[josdejong]

Thanks Glen, these are two really nice iterations of the function! Thanks to the comments it's quite clear what's going on.

The duplicated logic in the first approach of @samueltlg is indeed also not ideal, it's worth trying to find a solution without this duplication.

There is one thing that I do not fully understand in your second iteration: why is the second reduce in
return [sofar.reduce(...)] needed? I do not expect issues here, but loops inside loops is something I try to avoid generally since performance can get bad. Is this second reduce replaceable with just an operation on the last two entries or sofar maybe? Or will rewriting this again result in your first iteration?

[gwhitney]

why is the second reduce in
return [sofar.reduce(...)] needed?

It is because at this point, sofar may have exactly two or three entries, and they need to be reduced to a single Node. So this second reduce was the briefest way to express that. The last two lines of the outer reducing function could be replaced with something like:

sofar.push(_ensureNode(sofar.pop()))
const newtree = (sofar.length === 1) ? sofar[0] : makeNode(sofar)
return [makeNode([newtree, _ensureNode(next)])]

if you like that better. (Note here next is never pushed onto sofar so it has one or two entries, which are unified into a tree if there are two, and subsequently combined with next.)

[gwhitney]

(Note I just wrote that freehand, I haven't tested it, unlike the first two which passed all of the tests in develop -- neither have run with samueltlg's new tests or my recommended tests with various falsy constant operands.)

[samueltlg]

Thanks for your attempts/contributions @gwhitney. I like them both, but will later spend more time forming an opinion of the first attempt.

I spent considerable time breaking down the logic in your second attempt. The idea of returning a tuple for sofar was the idea I had initially! But it seems to work OK! I was inspired to try an attempt from scratch after seeing what you had done there with those two attempts, mainly to see how concise the solution could really be made (with a single reduce statement). Here's what I came up with:

  function foldOp (fn, args, makeNode, options) {
    // current uncollapsed constants encountered in args
    const currentConstants = []

    // reduce the new tree whilst simultaneously evaluating adjacent constants
    // ('sofar' is always either a tree or undefined (in the case of args. being solely constituted
    // by Fractions))
    let reduction = args.reduce((sofar, next) => {
      if (isNode(next)) {
        const constantsQty = currentConstants.length

        if (!constantsQty) {
          // collapse node into tree, or set as the first tree/node
          return sofar ? makeNode([sofar, next]) : next
        }

        const newTreeNodes = []
        if (sofar) newTreeNodes.push(sofar)

        // evaluate together all  constants/fractions, or return the sole current constant
        const evaluationResult = tryEval(fn, options, currentConstants)

        currentConstants.length = 0

        newTreeNodes.push(_ensureNode(evaluationResult), next)

        // Collapse previous tree (if present), uncollapsed constants (now simplified/eval'd and in
        // node form), and new/current node
        return newTreeNodes.reduce((a, b) => makeNode([a, b]))
      }

      currentConstants.push(next)

      // Either the current tree, or undefined
      return sofar
    }, undefined)

    // evaluate trailing constants, and collapse into tree (if formed)
    if (currentConstants.length) {
      const trailingConstResult = tryEval(fn, options, currentConstants)
      reduction = reduction ? makeNode([reduction, _ensureNode(trailingConstResult)]) : trailingConstResult
    }

    // Thr reduced tree (if args contains at least *one* node), or a Fraction
    return reduction

    /**
     * Attempt evaluation between all args provided.
     * If args is a one-length array, returns the only entry.
     *
     * @param {*} fn
     * @param {*} options
     * @param {*} args
     * @returns
     */
    function tryEval (fn, options, args) {
      if (args.length === 1) return args[0]

      try {
        const result = _eval(fn, args, options)
        return result
      } catch (ignoreandcontinue) {
        return undefined
      }
    }
  }

As you can see, it bears significant resemblance to gwhitney's second attempt, albeit with some refactoring (extracting eval. procedure, for example), and doing away with the tuple for sofar. Additionally, you may notice that sequences of adjacent constants are all evaluated in one call to _eval, which I don't think there should be a problem with?

And also, @josdejong, I have gone along with Glen's second-attempt reduce statement which has just been clarified. The same is also the case here; there should not be more than three iterations, and the use-case in both of our scenarios I think is always [node, constant, node].
(problematically also, I have not accounted for failure-calls to tryEval . And notably, exprs 'a + 0 + 7 + b' and 'a * 0 * 7 * b' seem to eval. fine in commutative + non-commutative contexts)

So I just attempted that for the challenge and the heck of it. One adv. is that it is also more readable similarly to these latest attempts. I'll have another look at all three later once I've had a rest from cognition! What do you two think? Is this one readable to you?

[gwhitney]

My apologies, I don't see how the latest proposal from samueltlg can be correct. In this version, if the eval in the attempt to fold constants throws an error, the tryEval returns undefined, which is then inserted in the tree, and the collected constants are thrown away. That's not a valid simplification; the old behavior is simply to preserve the constants that caused the throw as ConstantNodes. If there is not a unit test that exercises this behavior, one should be added. I stand by my recommendation of the following (with the second reduce unrolled for clarity/efficiency):

function foldOp (fn, args, makeNode, options) {
    const first = args.shift()
    // In the following reduction, sofar always has one of the three following
    // forms: [NODE], [CONSTANT], or [NODE, CONSTANT]
    const reduction = args.reduce((sofar, next) => {
      if (!isNode(next)) {
        const last = sofar.pop()
        if (isNode(last)) {
          return [last, next]
        }
        // Two constants in a row, try to fold them into one
        try {
          sofar.push(_eval(fn, [last, next], options))
          return sofar
        } catch (ignoreandcontinue) {
          sofar.push(last)
          // fall through to Node case
        }
      }
      // Encountered a Node, or failed folding --
      // collapse everything so far into a single tree:
      sofar.push(_ensureNode(sofar.pop()))
      const newtree = (sofar.length === 1) ? sofar[0] : makeNode(sofar)
      return [makeNode([newtree, _ensureNode(next)])]
    }, [first])

    if (reduction.length === 1) {
      return reduction[0]
    }
    // Might end up with a tree and a constant at the end:
    return makeNode([reduction[0], _toNode(reduction[1])])
  }

[samueltlg]

My apologies, I don't see how the latest proposal from samueltlg can be correct. In this version, if the eval in the attempt to fold constants throws an error, the tryEval returns undefined, which is then inserted in the tree, and the collected constants are thrown away. That's not a valid simplification; the old behavior is simply to preserve the constants that caused the throw as ConstantNodes. If there is not a unit test that exercises this behavior, one should be added. I stand by my recommendation of the following (with the second reduce unrolled for clarity/efficiency):

Hi @gwhitney. Indeed, your solution is fine. I thought that I would just have an attempt from scratch after coming across your two approaches (for my second attempt/first attempt at a single reduce statement, I left the existing code in place as much as I could, and only attempted to 'add' code, whilst this time inspired by what you had done, I was intrigued to see how it would turn out from a total re-write).

And I also remark in the comment that my attempt does not account for failure of eval. of constants; I'm sure that with some slight modification it could be made to work, though.

In any case, at minimum we have some working solutions now!

edit; and also, I can see that perhaps my recent version is not correctable; i.e. it is not sensible to evaluate multiple Fraction instances all in one go without anticipating that an error may be thrown? Is it known if there are many cases for this? (should not be if 'Infinity' comes into the picture?)

[josdejong]

It is because at this point, sofar may have exactly two or three entries, and they need to be reduced to a single Node.

Ah, now I get it. I think both options are fine. In case of using reduce an explanatory comment on top could be helpful.

@samueltlg which approach do you like most? I have a slight preference for Glen's latest proposal, but I don't have a strong preference here, so we can go with the approach you like most I think.

[samueltlg]

@samueltlg which approach do you like most? I have a slight preference for Glen's latest proposal, but I don't have a strong preference here, so we can go with the approach you like most I think.

I do like my latest for the readability, but @gwhitney 's is better I believe for conciseness, encapsulation, and is overall a little more sensible procedurally probably (not to mention also the current flaws in mine). So with that said, well done to all and let us go with that !

[josdejong]

Sounds good Samuel. Can you update the PR accordingly and let us know when ready? (maybe add a couple of unit tests to for the discussed edge cases)

[samueltlg]

Sounds good Samuel. Can you update the PR accordingly and let us know when ready? (maybe add a couple of unit tests to for the discussed edge cases)

Great! Could you please offer an example of a test where evaluation (between Fractions?) fails? Probably a good idea to have one or two in there to confirm everything is ok in these cases with the new code, and I am unaware of how to bring this about?

[gwhitney]

Could you please offer an example of a test where evaluation (between Fractions?) fails?

Not 100% sure. One way is to set config.number to bignumber in a mathjs instance that doesn't have bignumbers, but that sounds like a pain to set up. Maybe something like 7 + 'foo' or 3 / 0 (but I don't know whether the latter ever makes it to foldOp) or these same sorts of things embedded in larger expressions. Jos, do you happen to know any examples of why the try-catch for the operation on constants throwing an error was put in?

[gwhitney]

Well, 3/0 simplifies to Infinity, so that doesn't throw an error. But I think 7+'foo' is a bona fide example. In fact in develop right now without any context, 2 + 7 + 'foo' + 3 + 8 simplifies to 9 + 'foo' + 3 + 8 which is arguably a bug -- if it's gonna collapse the first two operands to 9, it oughta collapse the last two to 11. So I would just use some examples like that, unless Jos can remember other cases.

[samueltlg]

Well, 3/0 simplifies to Infinity, so that doesn't throw an error. But I think 7+'foo' is a bona fide example. In fact in develop right now without any context, 2 + 7 + 'foo' + 3 + 8 simplifies to 9 + 'foo' + 3 + 8 which is arguably a bug -- if it's gonna collapse the first two operands to 9, it oughta collapse the last two to 11. So I would just use some examples like that, unless Jos can remember other cases.

Great, I shall just go with that example then!

[samueltlg]

Just pushed up a working single commit. A couple things:

• I included the updated built AUTHORS file, please remove if it is not supposed to be associated
• @gwhitney has identified a case of incomplete simplification output in typical (commutative) contexts, which I have commented out as a possible 'todo' in simplifyConstant.js. (specifically, '2 + 7 + "foo" + 3 + 8' results in 9 + "foo" + 11 rather than 20 + "foo")

Also, not sure if you wish to close the associated issue of this PR, bearing in mind that there is still contributions left to be made ? ( on that note, I shall initiate further discussion there promptly :) )

[gwhitney]

OK, I made a couple of suggestions on the tests, which you can implement or not as you see fit. Let us know. Also, I totally agree that this will not fully resolve #2825 so that issue should not be closed when this is merged.

[samueltlg]

Just updated according to @gwhitney's suggestions, should be all ok now! (although one may wish to add a test (or two) on the point which this lengthy comment is covering in this commit, depending on your preferences).
Notice the definition of multi-arg expressions which I have been going along with in the writing of these tests (as clarified in the commentary). It may or may not be desirable, to add tests such as the following in either the same or an additional test/describe block, considering that tests of this type for a non-commutative context are absent in this file:

/*
 * 'control' tests (non-commutative context)
*/
testSimplifyConstant('2 + 3 - 4 + 5 - 6', '0', undefined, opts)
testSimplifyConstant('1 + 2 * 3 - 4 / 5', '1', undefined, opts)

[josdejong]

Thanks Samuel, looks good! Let's merge after wrapping up this last inline conversation.

[josdejong]

@samueltlg can you please run the npm run update-authors script once more and commit the latest version of AUTHORS? There is a small merge conflict at this moment 😅 .

[samueltlg]

@samueltlg can you please run the npm run update-authors script once more and commit the latest version of AUTHORS? There is a small merge conflict at this moment 😅 .

Should be good now!

[josdejong]

👌 thanks

[josdejong]

Published now in v11.4.0, thanks again Samuel 👍