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
Open

#76 - Better break-up fraction. #158

Show file tree
Hide file tree
Changes from 3 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
65 changes: 48 additions & 17 deletions lib/checks/canFindDenominatorInNumerator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const Node = require('../node');
// (2x+1)/(2x+3) -> (2x + 3)/(2x + 3) - 2/(2x + 3) -> 1 - 2/(2x + 3)
// e.g. (2x+1)/(2x^2 + 3) -> False
function canFindDenominatorInNumerator(node) {
if (!Node.Type.isOperator(node) || node.op !== '/' ) {
if (node.op !== '/' ) {
return false;
}
if (node.args.length !== 2) {
Expand All @@ -20,38 +20,69 @@ function canFindDenominatorInNumerator(node) {
if (Node.Type.isParenthesis(denominator)) {
denominator = denominator.content;
}
if (!(numerator.op === '+' || numerator.op === '-' ||
denominator.op === '+' || numerator.op === '-')) {

let n_args_length;
Copy link
Contributor

Choose a reason for hiding this comment

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

in JavaScript variable names tend to be camelCase and not underscore_like_this

(sorry - you were doing that before and I didn't comment on it)

// If numerator is '*' op, it signifies a single 'ax', should be assigned a
// length of 1
if ('args' in numerator && numerator.op !== '*') {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it !==? the comment makes it sound like you're checking for *

(if it is a typo, see if you can easily add a test that would fail on this typo)

Copy link
Author

@eltonlaw eltonlaw May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ah yeah good catch, it should be ===

EDIT: Actually, I remember why this is here now. Originally the two test cases 2x/(x+4) and (2x)/(2x +2) were failing because they had an 'args' key value: a node for 2 and a node for x. In these scenarios I want the n_args_length to equal 1. So this check is to ensure that only numerators with an 'args' key value AND a numerator.op of '+' or '-' are assigned a value of numerator.args.length. I changed it to check for that instead now.

Copy link
Contributor

Choose a reason for hiding this comment

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

great - make sure the comment explains why (not just what - so not just saying what the if statement is looking for, but an example like what you showed me so people know why you're checking that)

n_args_length = numerator.args.length;
}
else {
n_args_length = 1;
}
let d_args_length;
if ('args' in denominator) {
d_args_length = denominator.args.length;
}
else {
d_args_length = 1;
}

// If numerator isn't length 2 or length 1 with a polynomial return false
if (!(n_args_length === 2)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do !== (and same for other places you do this)

if (!(n_args_length === 1 || Node.Type.isConstant(numerator))) {
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 you could turn this all into one if statement instead of two

Copy link
Contributor

Choose a reason for hiding this comment

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

also you should be checking for polynomial, not constant, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

did you also check that the first argument of the numerator/denominator is a polynomial term?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it should be a check for polynomial, originally I wanted to put isSymbol() there, but it doesn't work on values that have a coefficient: ex. Node.Type.isSymbol('2x') evaluates to false, so i figured the logical positive would work instead: !Node.Type.isConstant('2x') evaluates to true.

Also, I don't check that the first argument of the numerator/denominator is a polynomial term because I assume that if we've already evaluated through and there's a second arg in the numerator/denominator that's a constant, the only thing left in the first args MUST be a polynomial, cause the other simplifiers would have added like terms already. Do you want me to explicitly write !Node.Type.isConstant(denominator.arg[0])?

Copy link
Contributor

Choose a reason for hiding this comment

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

there's a isPolynomialTerm (or something similar) defined somewhere - I think that's what you want. You could actually probably use it above when your'e trying to avoid the ax case (just make sure it's not a poly term)

!Node.Type.isConstant('2x') isn't rigorous enough because there are a bunch (5?) different types of node types - you can look in Node/Type to see them all

a counterexample to your assumption - what if it was 5/(2^x + 3)?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right. Oh wow, I didn't see isPolynomialTerm. Shoot, sorry for missing that. On a kind of unrelated note, to me it would make more sense if isPolynomialTerm was in the Type file instead, what do 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.

yeah that would be good to add there if it's easy! probably not instead, since it's nice to have the logic within polynomial term code, but if you could reference it in Type for now that'd be awesome

Copy link
Author

Choose a reason for hiding this comment

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

uhh, actually maybe not. If we require 'PolynomialTerm' from 'Type', then they're referencing each other. So the only way to put it in there is if we just have two copies. So yeah maybe screw it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh yeah okay I guess that's why that wasn't there then - ah well

return false;
}
}
// Function doesn't support denominators with args > 2
// Tf d_args_length < 2 the normal functionality already covers it
Copy link
Contributor

Choose a reason for hiding this comment

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

Tf -> If ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I forget - what's normal functionality?

also is d_args_length < 2 the same as equal to 1? because that'd be clearer

Copy link
Author

Choose a reason for hiding this comment

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

normal functionality is just taking each arg of the numerator and putting a denominator under it. So (ax^2 +bx +c)/d decomposes to ax^2/d +bx/d + c/d

Yeah it's the same as 1, I'll change it

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 that example in the comment just so people know what you mean?

Copy link
Author

Choose a reason for hiding this comment

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

okay i also added another two lines of explanation to try to make it a bit more clear, let me know if you think it adds to the confusion or helps.

if (!(d_args_length === 2)) {
return false;
}
if (denominator.op !== '+') {
if (!(denominator.op === '+' || denominator.op === '-')) {
return false;
}

// Check if numerator's second argument is a constant if numerator has two arguments
if (n_args_length === 2) {
if (!(Node.Type.isConstant(numerator.args[1]))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the parens around Node.Type(.....)

Copy link
Author

Choose a reason for hiding this comment

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

oh true!

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

if (!(numeratorFirstTerm.getSymbolName() === 'x' && denominatorFirstTerm.getSymbolName() === 'x')) {
// 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;
}

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]));
});