Better negative fractions #1
Conversation
lib/node/Type.js
Outdated
| if (NodeType.isOperator(node, '/')) { | ||
| return node.args.every(n => NodeType.isConstant(n, allowUnaryMinus)); | ||
| } | ||
| else if (allowUnaryMinus && NodeType.isUnaryMinus(node)) { |
There was a problem hiding this comment.
This is in the check isConstantFraction
for context, when there are no variables in an equation, solveConstantEquation gets called in solveEquation/stepThrough
this in turn led to
let rNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.rightNode, true)
let lNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.leftNode, true)
if (!lNodeIsConstant || !rNodeIsConstant) {
throw Error('Expected both nodes to be constants, instead got: ' +
equation.ascii());
}
Anywho, the issue is that -⅓ was returning false for this check because it was a urnary-minus paired with a fraction.
lib/solveEquation/stepThrough.js
Outdated
| let rNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.rightNode, true) | ||
| let lNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.leftNode, true) | ||
|
|
||
| if (!lNodeIsConstant || !rNodeIsConstant) { |
There was a problem hiding this comment.
this was just me re-arranging stuff for clarity
| const arg = flattenOperands(node.args[0]); | ||
| // since arg is changed when it is negated, clone it before negation to avoid | ||
| // modifiying original reference | ||
| const arg = flattenOperands(node.args[0].cloneDeep()); |
There was a problem hiding this comment.
Here we find the real pain. Note how the arg gets negated in the next line. When this is not cloned, it ends up messing up with the original node in stepThrough.
if you boot up your node console and try solving -((1)/(3))=((-1)/(3)) you'll see that on the left side an extra negative gets added! gasp! this was such a pain to track down.
For background, this was occuring in simplifyExpression/stepThrough in the step function
see L47:
console.log(node.toString()) // my logger
nodeStatus = step(node);
console.log(node.toString()) // my logger
gasp!
lib/util/removeUnnecessaryParens.js
Outdated
| else if (Node.Type.isUnaryMinus(node)) { | ||
| const content = node.args[0]; | ||
| let content = node.args[0]; | ||
|
|
||
| if (Node.Type.isParenthesis(content)) { | ||
| content = removeUnnecessaryParensInParenthesisNode(content) | ||
| } |
There was a problem hiding this comment.
here the assumption that I'm working with is that we can remove unnecessary parentheses from the arg of a unary minus. I don't see why we couldn't?
| } | ||
| const coeffNode = node.args[0]; | ||
| if (!NodeType.isConstantOrConstantFraction(coeffNode)) { | ||
| if (!NodeType.isConstantOrConstantFraction(coeffNode, true)) { |
There was a problem hiding this comment.
Wasn't allowing unary minus as a coefficient
No description provided.