-
Notifications
You must be signed in to change notification settings - Fork 276
#76 - Better break-up fraction. #158
base: master
Are you sure you want to change the base?
Conversation
This is only solves the initial case posed of (2x+3)/(2x+2). The output is now (2x+2)/(2x+2) + (1)/(2x+2).
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) |
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 think you mean - 3/ (5 + x) *.
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.
Whoops, yeah haha
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.
so then you'd also want to say add/subtract ^_^
if (numerator.op !== '+') { | ||
return false; | ||
} | ||
if (!('args' in denominator)) { |
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'm not understanding what this part does. Can you explain?
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.
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.
EDIT: actually I think with this we probably need to check that there's some sort of subtraction or addition somewhere in the fraction, so maybe set it to try to catch any of those scenarios. What do you think?
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.
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.
actually I think with this we probably need to check that there's some sort of subtraction or addition somewhere in the fraction, so maybe set it to try to catch any of those scenarios. What do you think?
I think the best way to check this is to make sure that the numerator and denominator aren't equal. if they're both x
then they're equal and there's nothing to do. but also if they're both x+1
then this step doesn't apply, we can just cancel (that step's code is elsewhere). How's that sound?
denominator = denominator.content; | ||
} | ||
|
||
if (numerator.op !== '+') { |
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.
Try adding the case where the numerator has minus operation too.
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.
looking good! thanks for working on this, a nice small change that'll add lots of teaching power to mathsteps :D
I left some comments for stuff to think about to make this better
@@ -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 |
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 ^_^
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 |
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.
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+3) -> True
// because of the simplification (2x+1)/(2x+3) -> (2x + 3)/(2x + 3) - 2/(2x + 3) -> 1 - 2/(2x + 3)
if (numerator.op !== '+') { | ||
return false; | ||
} | ||
if (!('args' in denominator)) { |
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.
actually I think with this we probably need to check that there's some sort of subtraction or addition somewhere in the fraction, so maybe set it to try to catch any of those scenarios. What do you think?
I think the best way to check this is to make sure that the numerator and denominator aren't equal. if they're both x
then they're equal and there's nothing to do. but also if they're both x+1
then this step doesn't apply, we can just cancel (that step's code is elsewhere). How's that sound?
if (!('args' in denominator)) { | ||
return false; | ||
} | ||
const numeratorFirstTerm = new Node.PolynomialTerm(numerator.args[0]); |
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.
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 (1+x) / (x+2)
. So we might need to have a step somewhere that sorts, or otherwise iterate through the arguments to find this poly term
Also, where do you make sure they have the same exponent? (i.e. for now that it's always 1)
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) |
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.
so then you'd also want to say add/subtract ^_^
const newNumerator = []; | ||
|
||
// The constant value difference between the numerator and the denominator | ||
const numeratorConstant = parseInt(numerator.args[1].value); |
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 don't think you ever make sure that this is actually a constant - do a check either here or in canFindDenominatorInNumerator
or else you'll get an error here
if (denominatorParenRemoved) { | ||
denominator = Node.Creator.parenthesis(denominator); | ||
} | ||
} |
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 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)
test/checks/checks.test.js
Outdated
|
||
describe('canSimplifyPolynomialTerms denominator in numerator', function() { | ||
const tests = [ | ||
['(2x + 3)/(2x + 2)', true], |
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.
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 :)
@@ -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))'], |
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.
same here - try to think of edge cases. I can help you brainstorm if you dunno what to add
Almost completely covers all cases of (ax^1 + b) / (cx^1 + d). The only current holes in this function are for cases where 1) Constant and polynomial term are not ordered 2) When b is equal to 0 'checks/canFindDenonimatorInNumerator': The checks are more specific than before. 'breakUpNumeratorSearch/index.js': Included a multiplier variable to see how much times the denominator is in the numerator. Instead of using numerator.args[1] it's now the args[num_n-1] to get the last index, this way as long as it's sorted we'll always get the constant. 'checks/checks.test.js': More test cases, included falses 'breakUpNumeratorSearch/breakUpNumeratorSearch.test.js': More test cases
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.
sorry for the delay - was at a conference last weekend! I'll try to be more quick back and forth with you now
I think we're gonna have to think a bit harder about how to get this working properly (unless I missed something) so let me know if you want help thinking about it!
@@ -30,8 +30,8 @@ function breakUpNumerator(node) { | |||
const fractionList = []; | |||
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) | |||
// Check if we can add/substract a constant to make the fraction nicer |
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.
substract -> subtract
@@ -19,17 +20,42 @@ function canFindDenominatorInNumerator(node) { | |||
if (Node.Type.isParenthesis(denominator)) { | |||
denominator = denominator.content; | |||
} | |||
if (!(numerator.op === '+' || numerator.op === '-' || | |||
denominator.op === '+' || numerator.op === '-')) { |
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.
why do you have numerator.op === '-'
twice?
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.
whoops that should be denominator.op
@@ -19,17 +20,42 @@ function canFindDenominatorInNumerator(node) { | |||
if (Node.Type.isParenthesis(denominator)) { | |||
denominator = denominator.content; | |||
} | |||
if (!(numerator.op === '+' || numerator.op === '-' || |
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.
can you add a comment explaining why you check for these things and why these are the only conditions that make us keep going?
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.
Okay
denominator.op === '+' || numerator.op === '-')) { | ||
return false; | ||
} | ||
if (denominator.op !== '+') { |
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.
why do you have this condition? what about (2x + 2) / 2x
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 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.
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.
ooo true. Maybe just add a comment saying where the other case is handled then!
['(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))'], |
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.
why are these commented out?
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.
As mentioned earlier, I didn't know what to do without some sort of way to sort it.
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.
ah, cool - sorry for missing that
|
||
let numeratorFirstTerm; | ||
if (numerator.op === '+') { | ||
numeratorFirstTerm = new Node.PolynomialTerm(numerator.args[0]); |
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'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
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.
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).
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.
yeah sure, I'm down to chat on gitter
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.
sweet - okay I actually am busy all day tomorrow >.> (first day of work!!)
does Wed evening work for you?
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.
or can we chat asynchronously - I can try to think about solutions to this tomorrow
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.
uhh yeah lets just chat asynchronously then, no worries take your time, good luck!
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 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? @
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.
oh whoops didn't see this, sure sounds good!
return false; | ||
} | ||
|
||
if (!(numeratorFirstTerm.getSymbolName() === 'x' && denominatorFirstTerm.getSymbolName() === 'x')) { |
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 think it's better to just say they have the same symbol. Then if they share the symbol y
, it'd work too!
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.
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
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.
yeah I think just doing getSymbolName() === getSymbolName() would work!
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.
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?
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.
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 x
s and change them to y
s and it should also work (actually can you add a test for that? ^_^)
numeratorFirstTerm.getSymbolName() === denominatorFirstTerm.getSymbolName()
is what I was thinking
I think I incorporated all your feedback @evykassirer, as things are now is this okay to get merged? |
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.
sorry - had to leave some more comments
I think one thing that'd help with readability a lot (and help me review easier hehe) is make a clear list of the conditions that must be true for it to return true at the top of the function declaration - exactly what the things you're going to check for are. Having them all together like that will make it easier to see what you're describing - what do you think?
let n_args_length; | ||
// If numerator is '*' op, it signifies a single 'ax', should be assigned a | ||
// length of 1 | ||
if ('args' in numerator && numerator.op !== '*') { |
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.
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)
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.
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.
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.
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)
if (!(numerator.op === '+' || numerator.op === '-' || | ||
denominator.op === '+' || numerator.op === '-')) { | ||
|
||
let n_args_length; |
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.
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 isn't length 2 or length 1 with a polynomial return false | ||
if (!(n_args_length === 2)) { |
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.
you can do !==
(and same for other places you do this)
|
||
// If numerator isn't length 2 or length 1 with a polynomial return false | ||
if (!(n_args_length === 2)) { | ||
if (!(n_args_length === 1 || Node.Type.isConstant(numerator))) { |
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 think you could turn this all into one if statement instead of two
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.
also you should be checking for polynomial, not constant, right?
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.
did you also check that the first argument of the numerator/denominator is a polynomial term?
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.
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])?
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.
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)
?
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.
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?
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.
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
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.
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.
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.
ahh yeah okay I guess that's why that wasn't there then - ah well
} | ||
} | ||
// Function doesn't support denominators with args > 2 | ||
// Tf d_args_length < 2 the normal functionality already covers it |
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.
Tf -> If ?
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 forget - what's normal functionality
?
also is d_args_length < 2
the same as equal to 1? because that'd be clearer
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.
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
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.
can you give that example in the comment just so people know what you mean?
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.
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.
|
||
// 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]))) { |
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.
you don't need the parens around Node.Type(.....)
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.
oh true!
return false; | ||
} | ||
|
||
if (!(numeratorFirstTerm.getSymbolName() === 'x' && denominatorFirstTerm.getSymbolName() === 'x')) { | ||
// Check that the symbols are the same, Ex. (x+1)/(y+1) would not pass |
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 love the example in the comment :D
I'm at GoogleIO this week and can't take much time to review your code - but I replied to some comments and will actually look at the diff on the weekend thanks for your patience! soon these rules will be easier to write hopefully - someone's working on a cleaner way to describe them right now :) (if you want to see more, check out https://github.com/semantic-math/math-rules) |
@@ -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 |
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 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"
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 love that you ahve this summary here though - it'll be so helpful!
|
||
let numeratorArgsLength; | ||
// If numerator is '*' op, it signifies a single 'ax', should be assigned a | ||
// length of 2 |
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 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
['(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))'], | ||
// ['(2x)/(2x + 3)', '((2x + 3) / (2x + 3)) - 3 / (2x + 3)'], |
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.
if these tests pass, uncomment them
otherwise leave a TODO with what we'll need to do in the future to make them pass
['(2x + 3)/(2x + 2)', 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 |
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.
these comments are super helpful!
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.
Hey Elton!
Good job on this pull request! Apologies for taking so long to submit my own code review. Thanks for taking the time to clean up the code, write helpful comments and add test cases. One quick change and this should be good for merging. I see you commented out the test case 2x / (2x + 3)
. Why is that? Isn't it suppose to pass? Like Evy said, if it passes uncomment it and if not, add a TODO to handle the case in a future PR. This is a great start to fraction manipulation. Hopefully you can continue to add more support for handling fractions. You could probably start by finding a way to rearrange the terms in a polynomial based on exponent. This would allow you to fix the case you skipped. Anyways, good job! Push the small change and we should be good to go.
Hey @aliang8 and @evykassirer, I think the problem with 2x / (2x +3) is the following line in breakUpNumeratorSearch/index.js:
the second part numerator.op !== '+' evaluates to true so the no change node is returned. I was thinking about adding a part before this where it adds an op to add a constant node '0' to the numerator in the case of a single polynomial numerator. But idk, it might end up really fragile. What do you think we should do? |
Ah good call, I see where you're referring to. Maybe you could add in a check to see if the numerator is a single polynomial term or if it is just a constant and handle it accordingly inside that if statement. By doing this, we can restrict to only the cases that we handle. So in practice that if statement could be: |
What about if you check your own conditions first, then check |
hey @evykassirer @aliang8, I've been trying to debug this but I can't seem to identify the problem. I've been poking around but I just can't seem to make sense of what's going on. When I try to incorporate the new checks everything seems to blow up. Sometimes tests from addConstantFractions give out errors, other times a bunch of tests in solveEquation fail. I am baffled. Do you guys want to check it out? In the meantime, I just commented it out and left a TODO. |
fixes #76
This is only solves the initial case posed of (2x+3)/(2x+2). The output is now
(2x+2)/(2x+2) + (1)/(2x+2).