Math error/completion infrastructure

[aelnaiem]

Even if something is parseable as math, doesn't mean it's valid.

We want some way to say that we encountered an error in trying to provide steps for the query. The simplest example is a division by zero error, but other errors include trying to find the square root of nothing.

Currently, we stop iterating steps when we get a noChange NodeStatus. The simplest thing would be to provide an optional error field on a NodeStatus that can also be checked.

We should probably also have an error for when we can't parse something, or encounter a node we can't handle.

[evykassirer]

related to #29 and #33

cc @hmaurer

[hmaurer]

Yes, I think this makes sense. All errors (parsing, division by zero, etc) would then generate an "error" node status and you would stop the iteration at that point.

Question: what kind of data would the error carry? For a syntax error you would likely report the index of the character which could not be parsed as well as an error message spit out by the parser. For an error such as division-by-zero it would be nice to report which part of the expression caused the issue. I think this might be somewhat related to how change-groups currently work? @evykassirer

[kevinbarabash]

What action do we want to take when we encounter an error? Depending on the answer to that question it might be easier to throw an exception and catch it at the top level.

[evykassirer]

if it's obvious that there's an error at the beginning (parse error) then I think throwing an error makes sense or something at the top level

but if it's dividing by 0, that could only become clear after a few steps have already happened, and I think it'd make sense to go like this:

x / (2 - 2)
x / 0
error: divide by 0

Where you mention the error once you've gotten there. I think using change groups or something to point out what part of the expression has an error would be great!

Then at any point, if the NodeStatus object has an error, we don't continue to get any more steps

[kevinbarabash]

I agree that we should show all of the successful steps, but it would be nice if we could avoid passing an error object when doing the tree searches. I was thinking a try/catch around the the call to step, not sure if this is the right place for that.

[kevinbarabash]

@evykassirer were you think that we'd check if NodeStatus had an error here?

[evykassirer]

yeah - that's what I was thinking.

it might get a bit more complicated because we sometimes do a few simplifications in a row to get substeps before going back to that top level function, and we might want to return early in those places too if there was an error

[tkosan]

I think using the try/catch mechanism is a good way to handle all sources of errors. This is the way that errors are handled in MathPiper, and it works well. However, MathPiper exposes an evaluate API method through which all of its functionality is accessed. The top-level try/catch catches all sources of errors and then places this information into the object that it returns as a response to an evaluation. Since mathsteps has more than one function that is used to access its functionality, I am not sure where a single try/catch would be placed.

[tkosan]

@evykassirer tree positions are an effective and elegant notation for identifying the part of a tree where an error occurred. A short explanation of how tree positions work is present in the following document starting at the first use of the word "position":

The language of expression trees

I learned about tree positions when I first started studying Dr. Alan Bundy's research, and now I use them for all kinds of things in MathPiper.

[evykassirer]

@hmaurer would you be interested in working on this to finish up #29 ?

If you don't think you have time for it, that's also fine - let me know!

[hmaurer]

@evykassirer I am a bit busy at the moment but I'll take a look at it next week. How are we going to handle substeps? For example, if an error occurs in a substep, should we have an "error" property on both the step's NodeStatus and the substep? I am not sure how this would look like.

[hmaurer]

@tkosan I wish I had heard of this sooner! I am working on a project with similar issues and it took me a while to come up with exactly this approach (tree positions).

[evykassirer]

well, each substep is itself a NodeStatus, so we can just say that any NodeStatus can have an 'error' property.

So if an error happens, we'd want to return early from generating the list of substeps instead of continuing to try other operations. It might be a bit annoying to have to remember to do this anytime we generate substeps though - maybe there's a better way.

Also I think we'd have to be able to tell there was an error from the top level stop at the top level function, so we exit.

So either we bubble up the error property or (my preferred option) when checking if we should stop because of an error during a step (in the top level stepThrough), search through that step's substeps for errors as well.

does this make sense? feels a bit messy. let me know what you think

[evykassirer]

tree positions look neat - I'm not sure what their benefit over changeGroups are, or if changeGroups have benefits over tree positions.

I'm opening another issue to talk about this #142

[hmaurer]

Ok, so let's recap:

When an error occurs, the step would look like this:

{
    oldNode: oldNode,
    error: {
        errorType: 'DIVISION_BY_ZERO',
        treePosition: treePosition,
    }
}

Does this make sense? Or should we have a changeType in the case of an error, e.g. ChangeTypes.ERROR?

Regarding error propagation within the codebase, we would do as @tkosan suggested: throw a javascript object when an error occurs and catch those at the top-level. If the error thrown is one of the above (an object with an errorType property or something) we would generate the relevant step, otherwise we would re-throw the error.

[evykassirer]

good idea to check in

it should be similar to a NodeStatus, so it would still have a changeType (I think having one called ERROR is good) and a newNode and oldNode (which would just be the same, both the 'old' node)

I'd rather we implement tree positions separately from this change - right now you could probably just use changeGroups, or wait for tree positions to be added before making this change

I have a feeling that the try catch wouldn't work, because we recurse on subtrees and have to build the tree back up. If we use treePositions/changeGroups, we still have to bubble back up to generate the position from the node or the new tree with the changeGroup. Also if we're in the middle of generating substeps, we would lose some of those substeps if we just throw an error and catch it at the top level

does that make sense?

[hmaurer]

@evykassirer I don't think it makes sense to have a newNode in case of error. Also I am not sure about what you are saying regarding try catch. It might pose an issue with substeps, I'll check the code. It might be better to wait until tree positions (or similar) are implemented before working on this?

[evykassirer]

yeah, it's hard because generally we use newNode to print the step, and we'd want to print it saying there's an error probably

an alternative is to set newNode = null when changeType = ChangeTypes.ERROR, and then be sure to refer to the oldNode to see what's causing the error

we can chat about the try catch a bit more if you want - I'm finding it hard to describe it clearly, but maybe we could chat back and forth a bit more on gitter or here and we can get on the same page and figure that out. you could also just play around with it and see what comes up

I think it would be fine to implement everything except the tree position part, and add that in later once tree positions are added (if we decide to do that). We don't have to know where in the tree the error happened yet, just being able to catch it would be a sweet start.

It's also fine to wait for it to happen first - do you have a preference?

(if you want to wait, let's label this with "blocked")