Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.

Fix order dependence on cancel like terms #237

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+144 −13
Open

Fix order dependence on cancel like terms #237

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fbf4514
Expressions with polynomials divided by polynomials are now simplified
marpol2 Jan 16, 2018
6e10e9e
minor naming/style changes
marpol2 Jan 17, 2018
d2ec124
minor thought fail fix
marpol2 Jan 17, 2018
7d4515a
fixed...
marpol2 May 16, 2018
3782254
fixed
marpol2 May 16, 2018
3e2c97c
Merge branch 'master' into fix-order-dependence-on-cancel-like-terms
marpol2 May 24, 2018
b4888cd
replaced clone() with cloneDeep()
marpol2 Jun 11, 2018
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
56 changes: 52 additions & 4 deletions lib/simplifyExpression/fractionsSearch/addConstantFractions.js
View file
Original file line number Diff line number Diff line change
@@ -19,7 +19,8 @@ function addConstantFractions(node) {
if (!Node.Type.isOperator(node) || node.op !== '+') {
return Node.Status.noChange(node);
}
if (!node.args.every(n => Node.Type.isIntegerFraction(n, true))) {
if (!node.args.every(n => Node.Type.isIntegerFraction(n, true) ||
hasIntegerMultiplicationDenominator(n))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for readability, can you put Node.Type.isIntegerFraction(n, true) || hasIntegerMultiplicationDenominator(n) on the second line together? at first I read it as !node.args.every(n => Node.Type.isIntegerFraction(n, true)) || hasIntegerMultiplicationDenominator(n) (which I know doesn't make sense :p)

and similar for other things below - when you write something longer than a line, can you do the linebreak at the beginning of a parenthesis instead of in the middle of it? I'm pretty sure there's a style guide out there somewhere that better explains what I mean, but I can't find it. Let me know if how I described this is confusing

return Node.Status.noChange(node);
}
const denominators = node.args.map(fraction => {
@@ -47,6 +48,16 @@ function addConstantFractions(node) {
newNode = Node.Status.resetChangeGroups(status.newNode);
}

const newDenominators = newNode.args.map(fraction => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose to put this between steps 1B and 2A? I'm also wondering if steps 1A and 1B might break if the denominators aren't integers yet but instead still multiplication.

My intuition is to move it before we even start any of the adding fraction logic - but you've thought more about this problem than me so I'm curious what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

also wherever it goes, adding a comment before it explaining what the step is for would be very helpful :)

return fraction.args[1];
});
if (!newDenominators.every(denominator => hasEqualNumberOfArgs(denominator, newDenominators[0]))){
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why you do this check (would be useful as a comment for other people who are also curious!)

Copy link
Contributor

Choose a reason for hiding this comment

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

also why do you call it newDenominators? does it get mutated?

// Multiply out the denominators
status = evaluateDenominators(newNode);
substeps.push(status);
newNode = Node.Status.resetChangeGroups(status.newNode);
}

// 2A. Now that they all have the same denominator, combine the numerators
// e.g. 2/3 + 5/3 -> (2+5)/3
status = combineNumeratorsAboveCommonDenominator(newNode);
@@ -136,15 +147,15 @@ function makeCommonDenominator(node) {
if (missingFactor !== 1) {
const missingFactorNode = Node.Creator.constant(missingFactor);
const newNumerator = Node.Creator.parenthesis(
Node.Creator.operator('*', [child.args[0], missingFactorNode]));
Node.Creator.operator('*', [child.args[0], missingFactorNode]));
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep it 2 space indentation :)

(same for all the other 4 space indentation you did below)

const newDeominator = Node.Creator.parenthesis(
Node.Creator.operator('*', [child.args[1], missingFactorNode]));
Node.Creator.operator('*', [child.args[1], missingFactorNode]));
newNode.args[i] = Node.Creator.operator('/', [newNumerator, newDeominator]);
}
});

return Node.Status.nodeChanged(
ChangeTypes.COMMON_DENOMINATOR, node, newNode);
ChangeTypes.COMMON_DENOMINATOR, node, newNode);
}

function evaluateDenominators(node) {
@@ -169,4 +180,41 @@ function evaluateNumerators(node) {
ChangeTypes.MULTIPLY_NUMERATORS, node, newNode);
}

function hasIntegerMultiplicationDenominator(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these two functions are so readable 👌

if (!Node.Type.isOperator(node, '/')) {
return false;
}
var denominator;
Copy link
Contributor

Choose a reason for hiding this comment

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

let please :)

(as far as I know, you can just use let and const and var isn't necessary anymore - but if you know differently, let me know!)

if (Node.Type.isParenthesis(node.args[1])) {
denominator = node.args[1].content;
}
else {
denominator = node.args[1];
}
if (!Node.Type.isOperator(denominator, '*')) {
return false;
}
if (!denominator.args.every(n => (Node.Type.isConstant(n, true) ||
Number.isInteger(parseFloat(n.value))))){
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here with having Node.Type.isConstant(n, true) || Number.isInteger(parseFloat(n.value)) on the same line

return false;
}
return true;
}

function hasEqualNumberOfArgs(n1, n2){
let node1 = n1.cloneDeep();
let node2 = n2.cloneDeep();
node1 = Node.Type.isParenthesis(node1) ? node1.content : node1;
node2 = Node.Type.isParenthesis(node2) ? node2.content : node2;
let length1 = 1;
let length2 = 1;
if (node1.args) {
length1 = node1.args.length;
}
if (node2.args) {
length2 = node2.args.length;
}
return length1 === length2;
}

module.exports = addConstantFractions;
82 changes: 78 additions & 4 deletions lib/simplifyExpression/fractionsSearch/cancelLikeTerms.js
View file
Original file line number Diff line number Diff line change
@@ -55,8 +55,9 @@ function cancelLikeTerms(node) {
// away because we always adjust the exponent in the numerator)
else if (isMultiplicationOfTerms(numerator) &&
!isMultiplicationOfTerms(denominator)) {
const numeratorArgs = Node.Type.isParenthesis(numerator) ?
numerator.content.args : numerator.args;
const prioritizedNumerator = prioritizeEqualConstantArgs(numerator, denominator);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here too to explain what this step does for people scanning the code here?

const numeratorArgs = Node.Type.isParenthesis(prioritizedNumerator) ?
prioritizedNumerator.content.args : prioritizedNumerator.args;
for (let i = 0; i < numeratorArgs.length; i++) {
const cancelStatus = cancelTerms(numeratorArgs[i], denominator);
if (cancelStatus.hasChanged) {
@@ -93,8 +94,9 @@ function cancelLikeTerms(node) {
// e.g. x / (x^2*y) => x^(1-2) / y
else if (isMultiplicationOfTerms(denominator)
&& !isMultiplicationOfTerms(numerator)) {
const denominatorArgs = Node.Type.isParenthesis(denominator) ?
denominator.content.args : denominator.args;
const prioritizedDenominator = prioritizeEqualConstantArgs(denominator, numerator);
const denominatorArgs = Node.Type.isParenthesis(prioritizedDenominator) ?
prioritizedDenominator.content.args : prioritizedDenominator.args;
for (let i = 0; i < denominatorArgs.length; i++) {
const cancelStatus = cancelTerms(numerator, denominatorArgs[i]);
if (cancelStatus.hasChanged) {
@@ -125,6 +127,43 @@ function cancelLikeTerms(node) {
numerator.content.args : numerator.args;
const denominatorArgs = Node.Type.isParenthesis(denominator) ?
denominator.content.args : denominator.args;

const likeTerms = getIndicesOfFirstTwoLikeTerms(numeratorArgs, denominatorArgs);
if (likeTerms){
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a distinct case from the code below - as in, either this code block will execute or the one below it will

to make that more clear, what do you think about adding a comment explaining this case, and then another comment for the case below?

maybe this one can be 4A the one below 4B, since they're both a subset of 4

const cancelStatus = cancelTerms(numeratorArgs[likeTerms.numeratorIndex],
denominatorArgs[likeTerms.denominatorIndex]);
if (cancelStatus.hasChanged) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh woops the distinct case is from here on

but wait, in what case would it not cancel if there were indeed like terms?

if (cancelStatus.numerator) {
numeratorArgs[likeTerms.numeratorIndex] = cancelStatus.numerator;
}
// if the cancelling out got rid of the numerator node, we remove it
Copy link
Contributor

Choose a reason for hiding this comment

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

ah man, I'm pretty sad this logic is mostly copy pasted 3 times in this file now :( but I remember last time I copied it, I couldn't figure out how to cleanly separate out this logic

I'll take a look again in a bit to think about it again, though feel free to take a stab at it first

// from the list
else {
numeratorArgs.splice(likeTerms.numeratorIndex, 1);
// if the numerator is now a "multiplication" of only one term,
// change it to just that term
if (numeratorArgs.length === 1) {
newNode.args[0] = numeratorArgs[0];
}
}
if (cancelStatus.denominator) {
denominatorArgs[likeTerms.denominatorIndex] = cancelStatus.denominator;
}
// if the cancelling out got rid of the denominator node, we remove it
// from the list
else {
denominatorArgs.splice(likeTerms.denominatorIndex, 1);
// if the denominator is now a "multiplication" of only one term,
// change it to just that term
if (denominatorArgs.length === 1) {
newNode.args[1] = denominatorArgs[0];
}
}
return Node.Status.nodeChanged(
ChangeTypes.CANCEL_TERMS, node, newNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can all be one line and it won't be too long
(oh I see you copied it from what I did below - my bad then xD you can change it everywhere if you want, or just keep it like this)

}
}

for (let i = 0; i < numeratorArgs.length; i++) {
for (let j = 0; j < denominatorArgs.length; j++) {
const cancelStatus = cancelTerms(numeratorArgs[i], denominatorArgs[j]);
@@ -414,4 +453,39 @@ function cancelCoeffs(numerator, denominator){
return new CancelOutStatus(newNumerator, newDenominator, true);
}

function prioritizeEqualConstantArgs(multiplicationNode, compareNode){
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space between ) { (and everywhere else you do it)

I'm sad the linter doesn't catch this - something to add to the linter! (I just made a new issue)

Copy link
Contributor

Choose a reason for hiding this comment

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

can you give this function a docstring? wasn't immediately obvious to me what it did and why (from the function name)

const isParens = Node.Type.isParenthesis(multiplicationNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make this name a bit more specific? something like "isMultiplicationNodeParens"

let content = multiplicationNode;
if (isParens){
content = multiplicationNode.content;
}
const multiplicationFactors = content.args;
for (let i=0; i<multiplicationFactors.length; i++){
if (multiplicationFactors[i].equals(compareNode)){
const temp = multiplicationFactors[0];
multiplicationFactors[0] = multiplicationFactors[i];
multiplicationFactors[i] = temp;
break;
}
}
if (isParens){
multiplicationNode.content.args = multiplicationFactors;
}
else {
multiplicationNode.args = multiplicationFactors;
}
return multiplicationNode;
}

function getIndicesOfFirstTwoLikeTerms(numeratorArgs, denominatorArgs){
for (let i=0; i<numeratorArgs.length; i++){
for (let j=0; j<denominatorArgs.length; j++){
if (numeratorArgs[i].equals(denominatorArgs[j])){
return { numeratorIndex: i, denominatorIndex: j };
}
}
}
return null;
}

module.exports = cancelLikeTerms;
19 changes: 14 additions & 5 deletions lib/simplifyExpression/fractionsSearch/divideByGCD.js
View file
Original file line number Diff line number Diff line change
@@ -43,6 +43,12 @@ function divideByGCD(fraction) {
return Node.Status.noChange(fraction);
}

if (numeratorValue === denominatorValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌 nice

I'm assuming that before, for this case it still worked out but the steps looked really weird? just wanna make sure this code block is necessary

newNode = Node.Creator.constant(1);
return Node.Status.nodeChanged(
ChangeTypes.SIMPLIFY_FRACTION, fraction, newNode, true);
}

// STEP 1: Find GCD
// e.g. 15/6 -> (5*3)/(2*3)
let status = findGCD(newNode, gcd, numeratorValue, denominatorValue);
@@ -68,15 +74,18 @@ function findGCD(node, gcd, numeratorValue, denominatorValue) {
const gcdNode = Node.Creator.constant(gcd);
gcdNode.changeGroup = 1;

const intermediateNumerator = Node.Creator.parenthesis(Node.Creator.operator(
'*', [Node.Creator.constant(numeratorValue/gcd), gcdNode]));
let intermediateNumerator = Node.Creator.constant(numeratorValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment/example here (similar to the ones in other places in this file)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it's more clear if you do let intermediateNumerator;
and

else {
  intermediateNumerator = Node.Creator.constant(numeratorValue);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

and also yayy thanks for fixing this case, it was pretty gross/weird before how it added the 1

if (numeratorValue / gcd !== 1) {
intermediateNumerator = Node.Creator.parenthesis(Node.Creator.operator(
'*', [Node.Creator.constant(numeratorValue / gcd), gcdNode]));
}
const intermediateDenominator = Node.Creator.parenthesis(Node.Creator.operator(
'*', [Node.Creator.constant(denominatorValue/gcd), gcdNode]));
'*', [Node.Creator.constant(denominatorValue / gcd), gcdNode]));
Copy link
Contributor

Choose a reason for hiding this comment

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

I thiiiink we use / without a space on either side, but I actually can't remember so if you find examples of this in the codebase then go for it

newNode = Node.Creator.operator(
'/', [intermediateNumerator, intermediateDenominator]);
'/', [intermediateNumerator, intermediateDenominator]);

return Node.Status.nodeChanged(
ChangeTypes.FIND_GCD, node, newNode, false);
ChangeTypes.FIND_GCD, node, newNode, false);
}

// Returns a substep where the GCD is cancelled out of numerator and denominator