Skip to content

Better negative fractions #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
GabrielMahan wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Better negative fractions #1

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d01cc9
clone node to avoid double negative being added
GabrielMahan Jun 2, 2020
3aa6b47
add comment
GabrielMahan Jun 2, 2020
626d787
better handling of unaryMinus with parenthesis
GabrielMahan Jun 2, 2020
110675a
add in better check for isOverallConstant
GabrielMahan Jun 5, 2020
92728b3
add allow unary
GabrielMahan Jun 5, 2020
6a1924c
cleanup debuggers
GabrielMahan Jun 5, 2020
f21dc6c
spec examples
GabrielMahan Jun 5, 2020
b774b61
add in negCoeff check
GabrielMahan Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/node/Type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ NodeType.isConstantFraction = function(node, allowUnaryMinus=false) {
if (NodeType.isOperator(node, '/')) {
return node.args.every(n => NodeType.isConstant(n, allowUnaryMinus));
}
else if (allowUnaryMinus && NodeType.isUnaryMinus(node)) {
Copy link
Owner Author

@GabrielMahan GabrielMahan Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return NodeType.isConstantFraction(node.args[0])
}
else {
return false;
}
Expand Down
6 changes: 4 additions & 2 deletions lib/solveEquation/stepThrough.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,8 +260,10 @@ function solveConstantEquation(equation, debug, steps=[]) {
equation.leftNode = removeUnnecessaryParens(equation.leftNode);
equation.rightNode = removeUnnecessaryParens(equation.rightNode);

if (!Node.Type.isConstantOrConstantFraction(equation.leftNode, true) ||
!Node.Type.isConstantOrConstantFraction(equation.rightNode, true)) {
let rNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.rightNode, true)
let lNodeIsConstant = Node.Type.isConstantOrConstantFraction(equation.leftNode, true)

if (!lNodeIsConstant || !rNodeIsConstant) {
Copy link
Owner Author

@GabrielMahan GabrielMahan Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just me re-arranging stuff for clarity

throw Error('Expected both nodes to be constants, instead got: ' +
equation.ascii());
}
Expand Down
4 changes: 3 additions & 1 deletion lib/util/flattenOperands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ function flattenOperands(node) {
return node;
}
else if (Node.Type.isUnaryMinus(node)) {
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());
Copy link
Owner Author

@GabrielMahan GabrielMahan Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

const flattenedNode = Negative.negate(arg, true);
if (node.changeGroup) {
flattenedNode.changeGroup = node.changeGroup;
Expand Down
7 changes: 6 additions & 1 deletion lib/util/removeUnnecessaryParens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ function removeUnnecessaryParensSearch(node) {
return node;
}
else if (Node.Type.isUnaryMinus(node)) {
const content = node.args[0];
let content = node.args[0];

if (Node.Type.isParenthesis(content)) {
content = removeUnnecessaryParensInParenthesisNode(content)
}
Copy link
Owner Author

@GabrielMahan GabrielMahan Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


node.args[0] = removeUnnecessaryParensSearch(content);
return node;
}
Expand Down