Skip to content
This repository has been 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
Open

#76 - Better break-up fraction. #158

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aee08a1
Issue #76 - Better break-up fraction.
eltonlaw Apr 21, 2017
6135dad
add check for denonimator in numerator
eltonlaw Apr 21, 2017
322a247
Issue #76 - Better break-up fraction
eltonlaw May 4, 2017
3c1cac0
lint
eltonlaw May 4, 2017
1ccdc3b
added test cases, checks are more rigorous
eltonlaw May 15, 2017
a46849d
Merge branch 'master' into master
eltonlaw May 15, 2017
fd8ebf0
add comments
eltonlaw May 15, 2017
f779651
update
eltonlaw May 16, 2017
41e9223
squash line 55-56 into one if statement
eltonlaw May 16, 2017
308a90c
Add changes
eltonlaw May 21, 2017
d88605f
Add TODO's
eltonlaw May 27, 2017
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
78 changes: 60 additions & 18 deletions lib/checks/canFindDenominatorInNumerator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,21 @@ const Node = require('../node');
// 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
// - Check that the number of arguments in parent node is 2
// - Check that the number of numerator args is equal to 2 or 1. In the case
// - of 1, we need that node to have a symbol (so it can't be a constant)
// - Check that denominator has two args
// - Check that the denominator op is '+' or '-'
// - If the numerator has 2 args, check that the second arg is a constant node
// - Check if the denominator's second arg is a constant node
// - Check to see that the denominator and numerator both don't have exponents
// - Check to see that the denominator and numerator have the same symbol

function canFindDenominatorInNumerator(node) {
if (!Node.Type.isOperator(node) || node.op !== '/' ) {
if (node.op !== '/' ) {
return false;
}
if (node.args.length !== 2) {
Expand All @@ -20,41 +33,70 @@ function canFindDenominatorInNumerator(node) {
if (Node.Type.isParenthesis(denominator)) {
denominator = denominator.content;
}
if (!(numerator.op === '+' || numerator.op === '-' ||
denominator.op === '+' || numerator.op === '-')) {

let numeratorArgsLength;
// If numerator is '*' op, it signifies a single 'ax', should be assigned a
// length of 2
Copy link
Contributor

@evykassirer evykassirer May 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think a clearer way to do this check is to see if it's a polynomial term (and then length 1) or else if it has args make it numerator.args.length, else the length is 1

idk if that's more confusing actually - up to you

but you should fix your comment cause I don't think that's right anymore

if ('args' in numerator &&
(numerator.op === '+' || numerator.op === '-')) {
numeratorArgsLength = numerator.args.length;
}
else {
numeratorArgsLength = 1;
}
let denominatorArgsLength;
if ('args' in denominator) {
denominatorArgsLength = denominator.args.length;
}
else {
denominatorArgsLength = 1;
}
// If numerator args isn't length 2 or length 1 with a polynomial return false
if (numeratorArgsLength !== 2 &&
(!(numeratorArgsLength === 1 && !Node.Type.isConstant(numerator)))) {
return false;
}
if (denominator.op !== '+') {
// Function doesn't support denominators with args > 2
// If denominatorArgsLength = 1 the normal functionality already covers it
if (denominatorArgsLength !== 2) {
return false;
}

if (!(denominator.op === '+' || denominator.op === '-')) {
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 •
edited
Loading

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 if (numerator.op === '*') {
else {
numeratorFirstTerm = new Node.PolynomialTerm(numerator);
}

let denominatorFirstTerm;
if (denominator.op === '+') {
denominatorFirstTerm = new Node.PolynomialTerm(denominator.args[0]);
}
else if (denominator.op === '*') {
denominatorFirstTerm = new Node.PolynomialTerm(denominator);
}

if (!(numeratorFirstTerm)) {
// If an exponent exists (aka not x^1), return false
if (numeratorFirstTerm.getExponentNode() ||
denominatorFirstTerm.getExponentNode()) {
return false;
}
if (!(denominatorFirstTerm)) {
// 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;
}

if (!(numeratorFirstTerm.getSymbolName() === 'x' && denominatorFirstTerm.getSymbolName() === 'x')) {
return false;
}

return true;
}

Expand Down
2 changes: 2 additions & 0 deletions lib/checks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const canAddLikeTermPolynomialNodes = require('./canAddLikeTermPolynomialNodes');
const canFindDenominatorInNumerator = require('./canFindDenominatorInNumerator');
const canMultiplyLikeTermConstantNodes = require('./canMultiplyLikeTermConstantNodes');
const canMultiplyLikeTermPolynomialNodes = require('./canMultiplyLikeTermPolynomialNodes');
const canRearrangeCoefficient = require('./canRearrangeCoefficient');
const canSimplifyPolynomialTerms = require('./canSimplifyPolynomialTerms');
Expand All @@ -10,6 +11,7 @@ const resolvesToConstant = require('./resolvesToConstant');
module.exports = {
canAddLikeTermPolynomialNodes,
canFindDenominatorInNumerator,
canMultiplyLikeTermConstantNodes,
canMultiplyLikeTermPolynomialNodes,
canRearrangeCoefficient,
canSimplifyPolynomialTerms,
Expand Down
2 changes: 1 addition & 1 deletion lib/simplifyExpression/breakUpNumeratorSearch/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function breakUpNumerator(node) {
const fractionList = [];
let denominator = node.args[1];

// Check if we can add/substract a constant to make the fraction nicer
// 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;
Expand Down
19 changes: 16 additions & 3 deletions test/checks/checks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,23 @@ describe('canSimplifyPolynomialTerms addition', function() {

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],
['(5x + 3)/(4)', false],
['(2x)/(2x + 3)', true],
['(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]));
});