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

#76 - Better break-up fraction. #158

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
98 changes: 98 additions & 0 deletions lib/checks/canFindDenominatorInNumerator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
const Node = require('../node');

// Returns true if by adding a term you can simplify part of the function into
// an integer
// e.g. (2x+1)/(2x+3) -> True because of the following simplification
// (2x+1)/(2x+3) -> (2x + 3)/(2x + 3) - 2/(2x + 3) -> 1 - 2/(2x + 3)
// e.g. (2x+1)/(2x^2 + 3) -> False
// ==============================================================================
// CHECKS
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 it would be most clear if you explain what exactly it handles

maybe:

"conditions include being a fraction where the numerator is a sum of ...."

so you see you can include if it's a + node and what the first and second argument is etc into a much more concise language, and that's what I was hoping for.

"Check that the number of arguments in parent node is 2" is already implied in it being a fraction

e.g. several of your conditions can be combined into "the denominator has to be an addition/subtraction of a polynomial term and a constant term"

Copy link
Contributor

Choose a reason for hiding this comment

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

I love that you ahve this summary here though - it'll be so helpful!

// - Check for division in parent node
// - Numerator has to be addition/subtraction of a polynomial term to the power
// of 1 and a constant term, in that order OR a polynomial term to the
// power of 1
// - Denominator has to be addition/subtraction of a polynomial term to the power
// of 1 and a constant term, in that order.
// - Check to see that the denominator and numerator have the same symbol

function canFindDenominatorInNumerator(node) {
if (node.op !== '/' ) {
return false;
}
let numerator = node.args[0];
let denominator = node.args[1];
if (Node.Type.isParenthesis(numerator)) {
numerator = numerator.content;
}
if (Node.Type.isParenthesis(denominator)) {
denominator = denominator.content;
}

let numeratorArgsLength;
// If numerator has args, but it's just a polynomial term, length is 1
// Ex. 3x/2x+3 => numeratorArgsLength=1
if (Node.PolynomialTerm.isPolynomialTerm(numerator)) {
numeratorArgsLength = 1;
}
// If numerator has args and args are two seperate values length is 2
// Ex. 3x+4/2x+3 => numeratorArgsLength=2
else if (numerator.op === '+' || numerator.op === '-') {
numeratorArgsLength = numerator.args.length;
}
// If numerator doesn't have args and isn't a polynomial term, there's
// nothing the added functionality can do
// Ex. 3/(2x + 3) => False
else {
return false;
}
let denominatorArgsLength;
if (denominator.op === '+' || denominator.op === '-') {
denominatorArgsLength = denominator.args.length;
}
// If denominator doesn't have args, it's length is 1. This case is already
// resolved by splitting the denominator into all the numerators
// Ex. (x + 3)/2x => x/2x + 3/2x
else {
return false;
}
// Function doesn't support denominators with args > 2
if (denominatorArgsLength !== 2) {
return false;
}
// Check if numerator's second argument is a constant if numerator has two arguments
if (numeratorArgsLength === 2) {
if (!Node.Type.isConstant(numerator.args[1])) {
return false;
}
}
// Check if denominator's second argument is a constant
if (!Node.Type.isConstant(denominator.args[1])) {
return false;
}
// Defines the first term depending on whether there's a coefficient value
// with the first term
let numeratorFirstTerm;
if (numerator.op === '+') {
numeratorFirstTerm = new Node.PolynomialTerm(numerator.args[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 concerned that we can just assume it'll be a polynomial term

(what if the second arg is a polynomial term? what if there are more than 2 args?)

Sorry this got more complicated haha. If you want, we can chat on gitter or something and figure out how to attack this

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this is what I wanted to talk about actually. One of the main issues is that it's not sorted, so I wanted to ask you all if we should add a sorting function which will run before this. Or if you don't see any merit in that, we could add some logic to find the highest polynomial term right into the function (or outside of it if you want).

Copy link
Author

Choose a reason for hiding this comment

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

yeah sure, I'm down to chat on gitter

Copy link
Contributor

Choose a reason for hiding this comment

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

sweet - okay I actually am busy all day tomorrow >.> (first day of work!!)

does Wed evening work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

or can we chat asynchronously - I can try to think about solutions to this tomorrow

Copy link
Author

Choose a reason for hiding this comment

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

uhh yeah lets just chat asynchronously then, no worries take your time, good luck!

Copy link
Contributor

@evykassirer evykassirer May 10, 2017

Choose a reason for hiding this comment

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

I think you could start with doing this rule only if there are exactly one or two args, the first is always a polynomial term, and the second if it exists is always a constant

i.e. come up with very limited cases and then test for them before going forward and do nothing if none of those cases work

and then after merging this you could add some more cases if they're easy enough

what do you think? @

Copy link
Author

Choose a reason for hiding this comment

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

oh whoops didn't see this, sure sounds good!

}
else {
numeratorFirstTerm = new Node.PolynomialTerm(numerator);
}
let denominatorFirstTerm;
if (denominator.op === '+') {
denominatorFirstTerm = new Node.PolynomialTerm(denominator.args[0]);
}
// If an exponent exists (aka not x^1), return false
if (numeratorFirstTerm.getExponentNode() ||
denominatorFirstTerm.getExponentNode()) {
return false;
}
// Check that the symbols are the same, Ex. (x+1)/(y+1) would not pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the example in the comment :D

if (!(numeratorFirstTerm.getSymbolName() ===
denominatorFirstTerm.getSymbolName())) {
return false;
}
return true;
}

module.exports = canFindDenominatorInNumerator;
2 changes: 2 additions & 0 deletions lib/checks/canSimplifyPolynomialTerms.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
const canAddLikeTermPolynomialNodes = require('./canAddLikeTermPolynomialNodes');
const canFindDenominatorInNumerator = require('./canFindDenominatorInNumerator');
const canMultiplyLikeTermPolynomialNodes = require('./canMultiplyLikeTermPolynomialNodes');
const canRearrangeCoefficient = require('./canRearrangeCoefficient');

// Returns true if the node is an operation node with parameters that are
// polynomial terms that can be combined in some way.
function canSimplifyPolynomialTerms(node) {
return (canAddLikeTermPolynomialNodes(node) ||
canFindDenominatorInNumerator(node) ||
canMultiplyLikeTermPolynomialNodes(node) ||
canRearrangeCoefficient(node));
}
Expand Down
2 changes: 2 additions & 0 deletions lib/checks/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const canAddLikeTermPolynomialNodes = require('./canAddLikeTermPolynomialNodes');
const canFindDenominatorInNumerator = require('./canFindDenominatorInNumerator');
const canMultiplyLikeTermConstantNodes = require('./canMultiplyLikeTermConstantNodes');
const canMultiplyLikeTermPolynomialNodes = require('./canMultiplyLikeTermPolynomialNodes');
const canRearrangeCoefficient = require('./canRearrangeCoefficient');
Expand All @@ -9,6 +10,7 @@ const resolvesToConstant = require('./resolvesToConstant');

module.exports = {
canAddLikeTermPolynomialNodes,
canFindDenominatorInNumerator,
canMultiplyLikeTermConstantNodes,
canMultiplyLikeTermPolynomialNodes,
canRearrangeCoefficient,
Expand Down
45 changes: 42 additions & 3 deletions lib/simplifyExpression/breakUpNumeratorSearch/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const canFindDenominatorInNumerator = require('../../checks/canFindDenominatorInNumerator');
const ChangeTypes = require('../../ChangeTypes');
const Node = require('../../node');
const TreeSearch = require('../../TreeSearch');
Expand Down Expand Up @@ -27,8 +28,47 @@ function breakUpNumerator(node) {
// At this point, we know that node is a fraction and its numerator is a sum
// of terms that can't be collected or combined, so we should break it up.
const fractionList = [];
const denominator = node.args[1];
numerator.args.forEach(arg => {
let denominator = node.args[1];

// Check if we can add/subtract a constant to make the fraction nicer
// fraction e.g. (2+x)/(5+x) -> (5+x)/(5+x) - 3/(5+x)
if (canFindDenominatorInNumerator(node)) {
let denominatorParenRemoved = false;
if (Node.Type.isParenthesis(denominator)) {
denominatorParenRemoved = true;
denominator = denominator.content;
}
const newNumerator = [];

// The constant value difference between the numerator and the denominator
const num_n = numerator.args.length;
const den_n = denominator.args.length;
const numeratorFirstTerm = new Node.PolynomialTerm(numerator.args[0]);
const denominatorFirstTerm = new Node.PolynomialTerm(denominator.args[0]);
const numeratorPolyCoeff = numeratorFirstTerm.getCoeffValue();
const denominatorPolyCoeff = denominatorFirstTerm.getCoeffValue();
const multiplier = numeratorPolyCoeff / denominatorPolyCoeff;

const numeratorConstant = parseInt(numerator.args[num_n-1].value) || 0;
const denominatorConstant = parseInt(denominator.args[den_n-1].value) || 0;
const addedConstant = numeratorConstant - (denominatorConstant * multiplier);

if (multiplier === 1) {
newNumerator.push(denominator);
}
else {
const multiplierNode = Node.Creator.constant(multiplier);
newNumerator.push(Node.Creator.operator('*', [multiplierNode, denominator]));
}
newNumerator.push(Node.Creator.constant(addedConstant));

numerator = newNumerator;

if (denominatorParenRemoved) {
denominator = Node.Creator.parenthesis(denominator);
}
}
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 it'd be best if we can break this up into two steps. the step you added, and the break up fraction that comes after (and then maybe we can group in the cancelling that comes after). and then we'd have substeps for breaking up the fraction.

what do you think? (let me know if you want me to explain more what this means)

numerator.forEach(arg => {
const newFraction = Node.Creator.operator('/', [arg, denominator]);
newFraction.changeGroup = 1;
fractionList.push(newFraction);
Expand All @@ -41,5 +81,4 @@ function breakUpNumerator(node) {
return Node.Status.nodeChanged(
ChangeTypes.BREAK_UP_FRACTION, node, newNode, false);
}

module.exports = search;
23 changes: 23 additions & 0 deletions test/checks/checks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,26 @@ describe('canSimplifyPolynomialTerms addition', function() {
];
tests.forEach(t => testCanCombine(t[0], t[1]));
});

describe('canSimplifyPolynomialTerms denominator in numerator', function() {
const tests = [
['(x+1)/(x-2)', true],
['(2x)/(x+4)', true],
['(x)/(x+4)', true],
['(x)/(2x+4)', true],
['(x+3)/(x)', false], // Normal breakup function already solves this
['(2x + 3)/(2x + 2)', true],
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 bunch more tests? especially a cases that should be false

you can use some of my comments above for edge cases to test for too :)

['(2x+3)/(2x)', false], // Normal breakup function already solves this
['(2x)/(2x + 2)', true],
['(5x + 3)/(4)', false], // Normal breakup function already solves this
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments are super helpful!

// Not supported yet
['(2x)/(2 + 2x)', false],
['(2 + 2x)/(3x + 4)', false],
['(x + 3)/(2x^2 + 5)', false],
['(3x^2 + 3)/(2x^2 + 5)', false],
['(5x^2 + 3)/(2x + 5)', false],
['(5x^2-4x + 3)/(2x + 5)', false],
['(-4x + 3)/(2x^2 + 5x +7)', false],
];
tests.forEach(t => testCanCombine(t[0], t[1]));
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ describe('breakUpNumerator', function() {
['(x+3+y)/3', '(x / 3 + 3/3 + y / 3)'],
['(2+x)/4', '(2/4 + x / 4)'],
['2(x+3)/3', '2 * (x / 3 + 3/3)'],
['(2x + 3)/(2x + 2)', '((2x + 2) / (2x + 2) + 1 / (2x + 2))'],
['(2x - 3)/(2x + 2)', '((2x + 2) / (2x + 2) - 5 / (2x + 2))'],
['(2x + 3)/(2x)', '(2x / (2x) + 3 / (2x))'],
['(3 + 2x)/(2x)', '(3 / (2x) + 2x / (2x))'],
['(4x + 3)/(2x + 2)', '(2 * (2x + 2) / (2x + 2) - 1 / (2x + 2))'],
// ['(2x)/(3 + 2x)', '((3 + 2x) / (3 + 2x) - 3 / (3 + 2x))'],
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these commented out?

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned earlier, I didn't know what to do without some sort of way to sort 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, cool - sorry for missing that

// ['(2x)/(2x + 3)', '((2x + 3) / (2x + 3)) - 3 / (2x + 3)'],
Copy link
Contributor

Choose a reason for hiding this comment

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

if these tests pass, uncomment them

otherwise leave a TODO with what we'll need to do in the future to make them pass

];
tests.forEach(t => testBreakUpNumeratorSearch(t[0], t[1]));
});