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 all 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
2 changes: 1 addition & 1 deletion lib/node/Term.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ Term.parseNode = function(node, baseNodeFunc, onlyImplicitMultiplication) {
throw Error('Expected two arguments to *');
}
const coeffNode = node.args[0];
if (!NodeType.isConstantOrConstantFraction(coeffNode)) {
if (!NodeType.isConstantOrConstantFraction(coeffNode, true)) {
Copy link
Owner Author

@GabrielMahan GabrielMahan Jun 5, 2020

Choose a reason for hiding this comment

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

Wasn't allowing unary minus as a coefficient

throw Error('Expected coefficient to be constant or fraction of ' +
'constants term, got ' + coeffNode);
}
Expand Down
10 changes: 10 additions & 0 deletions lib/node/Type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ NodeType.isConstantOrConstantFraction = function(node, allowUnaryMinus=false) {
}
};

NodeType.isOverallConstant = function (node) {
if (NodeType.isConstantOrConstantFraction(node, true)) {
return true
} else if (NodeType.isUnaryMinus(node)) {
return NodeType.isOverallConstant(node.args[0])
} else if (NodeType.isParenthesis(node)) {
return NodeType.isOverallConstant(node.content)
}
}

NodeType.isIntegerFraction = function(node, allowUnaryMinus=false) {
if (!NodeType.isConstantFraction(node, allowUnaryMinus)) {
return false;
Expand Down
7 changes: 3 additions & 4 deletions lib/solveEquation/stepThrough.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,10 @@ function solveConstantEquation(equation, debug, steps=[]) {

// If the left or right side didn't have any steps, unnecessary parens
// might not have been removed, so do that now.
equation.leftNode = removeUnnecessaryParens(equation.leftNode);
equation.rightNode = removeUnnecessaryParens(equation.rightNode);
equation.leftNode = removeUnnecessaryParens(equation.leftNode, true);
equation.rightNode = removeUnnecessaryParens(equation.rightNode, true);

if (!Node.Type.isConstantOrConstantFraction(equation.leftNode, true) ||
!Node.Type.isConstantOrConstantFraction(equation.rightNode, true)) {
if (!Node.Type.isOverallConstant(equation.leftNode) || !Node.Type.isOverallConstant(equation.rightNode)) {
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
3 changes: 1 addition & 2 deletions lib/util/removeUnnecessaryParens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ function removeUnnecessaryParensSearch(node) {
return node;
}
else if (Node.Type.isUnaryMinus(node)) {
const content = node.args[0];
node.args[0] = removeUnnecessaryParensSearch(content);
node.args[0] = removeUnnecessaryParensSearch(node.args[0])
return node;
}
else {
Expand Down
4 changes: 3 additions & 1 deletion test/solveEquation/solveEquation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ describe('solveEquation for =', function () {
['((x)/(4))=4', 'x = 16'],
['(2x-12)=(x+4)', 'x = 16'],
['x+y=x+y', '0 = 0'],
['y + 2x = 14 + y', 'x = 7']
['y + 2x = 14 + y', 'x = 7'],
['-(x/2) = 4', 'x = -8'],
['-((1)/(3)) = ((-1)/(3))', '-(1/3) = -1/3']
// TODO: fix these cases, fail because lack of factoring support, for complex #s,
// for taking the sqrt of both sides, etc
// ['(x + y) (y + 2) = 0', 'y = -y'],
Expand Down
1 change: 1 addition & 0 deletions test/util/removeUnnecessaryParens.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ describe('removeUnnecessaryParens', function () {
['x + (12)', 'x + 12'],
['x + (y)', 'x + y'],
['x + -(y)', 'x - y'],
['((((1))))', '1'],
['((3 - 5)) * x', '(3 - 5) * x'],
['((3 - 5)) * x', '(3 - 5) * x'],
['(((-5)))', '-5'],
Expand Down