-
Notifications
You must be signed in to change notification settings - Fork 281
#76 - Better break-up fraction. #158
base: master
Are you sure you want to change the base?
Changes from 2 commits
aee08a1
6135dad
322a247
3c1cac0
1ccdc3b
a46849d
fd8ebf0
f779651
41e9223
308a90c
d88605f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be easiest to understand this function (from an outsider perspective) if you explain why it's true as well |
||
| // 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 !== '+') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try adding the case where the numerator has minus operation too. |
||
| return false; | ||
| } | ||
| if (!('args' in denominator)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not understanding what this part does. Can you explain?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line 23 is to check whether the numerator is the '+' op between nodes, so that's to check that there's an 'x + y' kind of thing going on, but now that you mention it, it's not actually required. cause we should be able to go from x/(x+1) -> (x+1)/(x+1) -1)/(x+1). I'll take this out. line 26 is in there because tests were failing by having a constant in the bottom, i think a constant in the bottom is resolved best the original way.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the best way to check this is to make sure that the numerator and denominator aren't equal. if they're both |
||
| return false; | ||
| } | ||
| const numeratorFirstTerm = new Node.PolynomialTerm(numerator.args[0]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. most of the time I think it'll be sorted so that the first term is the poly term, but I don't think this is guaranteed. you could have Also, where do you make sure they have the same exponent? (i.e. for now that it's always 1) |
||
| const denominatorFirstTerm = new Node.PolynomialTerm(denominator.args[0]); | ||
|
|
||
| return numeratorFirstTerm.getSymbolName() === denominatorFirstTerm.getSymbolName(); | ||
| } | ||
|
|
||
| module.exports = canFindDenominatorInNumerator; | ||
| 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'); | ||
|
|
@@ -27,8 +28,33 @@ 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 a constant to make the fraction nicer | ||
| // fraction e.g. (2+x)/(5+x) -> (5+x)/(5+x) + 3/(5+x) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you mean - 3/ (5 + x) *.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, yeah haha
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so then you'd also want to say add/subtract ^_^ |
||
| 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 numeratorConstant = parseInt(numerator.args[1].value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you ever make sure that this is actually a constant - do a check either here or in |
||
| const denominatorConstant = parseInt(denominator.args[1].value); | ||
| const addedConstant = numeratorConstant - denominatorConstant; | ||
|
|
||
| newNumerator.push(Node.Creator.constant(addedConstant)); | ||
| newNumerator.push(denominator); | ||
|
|
||
| numerator = newNumerator; | ||
|
|
||
| if (denominatorParenRemoved) { | ||
| denominator = Node.Creator.parenthesis(denominator); | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
@@ -41,5 +67,4 @@ function breakUpNumerator(node) { | |
| return Node.Status.nodeChanged( | ||
| ChangeTypes.BREAK_UP_FRACTION, node, newNode, false); | ||
| } | ||
|
|
||
| module.exports = search; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,3 +29,10 @@ 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], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) |
||
| ]; | ||
| tests.forEach(t => testCanCombine(t[0], t[1])); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ 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)', '(1 / (2x + 2) + (2x + 2) / (2x + 2))'], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - try to think of edge cases. I can help you brainstorm if you dunno what to add |
||
| ]; | ||
| tests.forEach(t => testBreakUpNumeratorSearch(t[0], t[1])); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's almost 80 characters here so it feels annoying to put it in two lines, but... 80 character max please ^_^