Skip to content
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.

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

Jump to bottom

#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 4 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 61 additions & 0 deletions lib/checks/canFindDenominatorInNumerator.js
View file
Open in desktop
@@ -0,0 +1,61 @@
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
function canFindDenominatorInNumerator(node) {
if (!Node.Type.isOperator(node) || node.op !== '/' ) {
return false;
}
if (node.args.length !== 2) {
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;
}
if (!(numerator.op === '+' || 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.

can you add a comment explaining why you check for these things and why these are the only conditions that make us keep going?

Copy link
Author

Choose a reason for hiding this comment

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

Okay

denominator.op === '+' || 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 do you have numerator.op === '-' twice?

Copy link
Author

Choose a reason for hiding this comment

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

whoops that should be denominator.op

return false;
}
if (denominator.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 do you have this condition? what about (2x + 2) / 2x

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking we don't need to use this extended version on it. The original breakUpNumerator function works fine for this case. But I think you've highlighted an interesting point, anyone reading the code would be confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

ooo true. Maybe just add a comment saying where the other case is handled then!

return false;
}

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

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 === '*') {
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)) {
return false;
}

if (!(numeratorFirstTerm.getSymbolName() === 'x' && denominatorFirstTerm.getSymbolName() === 'x')) {
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's better to just say they have the same symbol. Then if they share the symbol y, it'd work too!

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, I didn't think about the symbol being a different name. The reason I did it to equal 'x' was to restrict it to the case of x^1

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think just doing getSymbolName() === getSymbolName() would work!

Copy link
Author

@eltonlaw eltonlaw May 9, 2017 •
edited

Choose a reason for hiding this comment

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

wait I'm not really getting the use of getSymbolName() actually, what other values can the symbol even take, it seems like it always returns 'x'. I originally thought the getSymbolName would grab the exponent as well, but that's actually covered by getExponentNode.

Is getSymbolName just for when there are multiple variables? Cause that seems way out of the scope for now anyways.

Would it be better to check that denominatorFirstTerm.getExponentNode() === numeratorFirstTerm.getExponentNode() or check if they equal undefined? I guess we could keep the getSymbolName === getSymbolName there just for good measure. Is this correct or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

using getSymbolName makes sure the symbol in numerator and denominator are the same - i.e. both x or both y or whatever - you could take any example with xs and change them to ys and it should also work (actually can you add a test for that? ^_^)

numeratorFirstTerm.getSymbolName() === denominatorFirstTerm.getSymbolName() is what I was thinking

return false;
}

return true;
}

module.exports = canFindDenominatorInNumerator;
2 changes: 2 additions & 0 deletions lib/checks/canSimplifyPolynomialTerms.js
View file
Open in desktop
@@ -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
View file
Open in desktop
@@ -1,4 +1,5 @@
const canAddLikeTermPolynomialNodes = require('./canAddLikeTermPolynomialNodes');
const canFindDenominatorInNumerator = require('./canFindDenominatorInNumerator');
const canMultiplyLikeTermPolynomialNodes = require('./canMultiplyLikeTermPolynomialNodes');
const canRearrangeCoefficient = require('./canRearrangeCoefficient');
const canSimplifyPolynomialTerms = require('./canSimplifyPolynomialTerms');
Expand All @@ -8,6 +9,7 @@ const resolvesToConstant = require('./resolvesToConstant');

module.exports = {
canAddLikeTermPolynomialNodes,
canFindDenominatorInNumerator,
canMultiplyLikeTermPolynomialNodes,
canRearrangeCoefficient,
canSimplifyPolynomialTerms,
Expand Down
45 changes: 42 additions & 3 deletions lib/simplifyExpression/breakUpNumeratorSearch/index.js
View file
Open in desktop
@@ -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/substract a constant to make the fraction nicer
Copy link
Contributor

Choose a reason for hiding this comment

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

substract -> subtract

// 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;
10 changes: 10 additions & 0 deletions test/checks/checks.test.js
View file
Open in desktop
Expand Up @@ -29,3 +29,13 @@ describe('canSimplifyPolynomialTerms addition', function() {
];
tests.forEach(t => testCanCombine(t[0], t[1]));
});

describe('canSimplifyPolynomialTerms denominator in numerator', function() {
const tests = [
['(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],
];
tests.forEach(t => testCanCombine(t[0], t[1]));
});
7 changes: 7 additions & 0 deletions test/simplifyExpression/breakUpNumeratorSearch/breakUpNumeratorSearch.test.js
View file
Open in desktop
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]));
});