Conversation
| // e.g. 2 * 4x + 2*5 --> 8x + 10 (as part of distribution) | ||
| SIMPLIFY_TERMS: 'SIMPLIFY_TERMS', | ||
| // e.g. (nthRoot(x, 2))^2 -> nthRoot(x, 2) * nthRoot(x, 2) | ||
| EXPAND_EXPONENT: 'EXPAND_EXPONENT', |
There was a problem hiding this comment.
Can you add (2x + 3)^2 -> (2x + 3) (2x + 3) as an example here too? Just cause that might be the more common use case.
| } | ||
| } | ||
|
|
||
| // Expand a power node with a non-constant base |
| return Node.Status.noChange(node); | ||
| } | ||
|
|
||
| // If the base is an nthRoot node, it doesn't need the parenthesis |
There was a problem hiding this comment.
Definitely not a big deal, so just mentioning this to make you an even more amazing coder: to eliminate code repetition, you could instead do 1) expandedBase = base if nthRoot else node.args[0] 2) expandedNode = Node.creator.operator ... fill(expandedBase)
|
|
||
| describe('solveEquation errors', function() { | ||
| const tests = [ | ||
| ['( x + 2) ^ ( 2) - x ^ ( 2) = 4( x + 1)'] |
There was a problem hiding this comment.
Can we move this test case somewhere else now?
| // ['(nthRoot(x, 2))^3', 'nthRoot(x ^ 3, 2)'], | ||
| // ['3 * nthRoot(x, 2) * (nthRoot(x, 2))^2', '3 * nthRoot(x ^ 3, 2)'], | ||
| // TODO: fix when base has multiplication | ||
| // ['(nthRoot(x, 2) * nthRoot(x, 2))^2', 'x^2'], |
There was a problem hiding this comment.
Why doesn't this work? The inner part should just evaluate to x right (you fixed this in another recent PR), and then you have (x)^2?
There was a problem hiding this comment.
Not exactly, it evaluates to (nthRoot(x^2,2)^2 and then it does (nthRoot(x^2,2) * (nthRoot(x^2, 2)
There was a problem hiding this comment.
I see, so when we support nthRoot(x^2,2) -> x then it will work?
There was a problem hiding this comment.
We already support that. I think we're distributing before evaluating nthRoots so that's why we take that route.
There was a problem hiding this comment.
Ok I'm quite confused. Does this case work or not? If not, why not, and can you fix? If so, why is the test commented out?
There was a problem hiding this comment.
Not exactly. This is because distribution code runs before function (nthRoot) search. So we would do (nthRoot(x^2 , 2)^2 -> nthRoot(x^2, 2) * nthRoot(x^2, 2).
|
Really pleased with this, well done @aliang8 ! Great comments and test cases. And code was very readable. Just left a few things to address. |
| ['3 * (nthRoot(x, 2))^2', '3x'], | ||
| ['(nthRoot(x, 2))^6 * (nthRoot(x, 3))^3', 'x^4'], | ||
| ['(2x + 3)^2','4x^2 + 12x + 9'], | ||
| ['(x + 3 + 4)^2', 'x^2 + 14x + 49'], |
There was a problem hiding this comment.
if this does what #187 does can you add the tests from that PR here too?
There was a problem hiding this comment.
I did not because apparently there are no substeps for (x-2)^2 anymore. But I will add them into simplify.test.js.
There was a problem hiding this comment.
Wait, well shouldn't we have substeps for (x-2)^2? Is this a unary minus issue? If so I think we should fix it.
There was a problem hiding this comment.
(Just discussed over slack, this does actually work, we're fine)
|
@evykassirer once @aliang8 addresses my confusion above, can this be approved/merged? |
|
sorry was falling behind :( didn't get time to review this today. Figured I'd wait for your review to get resolved. If it's urgent feel free to merge, otherwise I might be able to get to it tomorrow before I head to work or latest after work |
evykassirer
left a comment
evykassirer
left a comment
There was a problem hiding this comment.
nvm looked anyways cause I felt bad but also it was small and very clean! looks good to me, the comments are just little cleanups if you want to do them
great work 🎉
| } | ||
| else if (Node.Type.isOperator(node)) { | ||
| else if (Node.Type.isOperator(node, '*')) { | ||
| return distributeAndSimplifyOperationNode(node); |
There was a problem hiding this comment.
you could rename this to distributeAndSimplifyMultiplication now
|
|
||
| if (!Node.Type.isFunction(base, 'nthRoot') && !Node.Type.isOperator(base, '+')) { | ||
| return Node.Status.noChange(node); | ||
| } |
There was a problem hiding this comment.
this code is so cleannnn ✨ so easy to read
| // If the base is an nthRoot node, it doesn't need the parenthesis | ||
| const expandedBase = Node.Type.isFunction(base, 'nthRoot') | ||
| ? base | ||
| : node.args[0]; |
There was a problem hiding this comment.
nit: might be a bit more readable to explicitly do Node.Creator(base)
| ['3 * (nthRoot(x, 2))^4', '3 * nthRoot(x, 2) * nthRoot(x, 2) * nthRoot(x, 2) * nthRoot(x, 2)'], | ||
| ['(nthRoot(x, 2) + nthRoot(x, 3))^2', '(nthRoot(x, 2) + nthRoot(x, 3)) * (nthRoot(x, 2) + nthRoot(x, 3))'], | ||
| ['(2x + 3)^2', '(2x + 3) * (2x + 3)'], | ||
| ['(x + 3 + 4)^2', '(x + 3 + 4) * (x + 3 + 4)'] |
There was a problem hiding this comment.
could be worth testing some cases that shouldn't expand here too
| // ['(nthRoot(x, 2))^3', 'nthRoot(x ^ 3, 2)'], | ||
| // ['3 * nthRoot(x, 2) * (nthRoot(x, 2))^2', '3 * nthRoot(x ^ 3, 2)'], | ||
| // TODO: fix when base has multiplication | ||
| // ['(nthRoot(x, 2) * nthRoot(x, 2))^2', 'x^2'], |
| // -> x * x -> x | ||
| ['(nthRoot(x, 2) * nthRoot(x, 2))^2', 'x^2'], | ||
| // TODO: fix nthRoot to evaluate nthRoot(x^3, 2) | ||
| // ['(nthRoot(x, 2))^3', 'nthRoot(x ^ 3, 2)'], |
There was a problem hiding this comment.
This test case also works? Why is it commented out?
| ['(nthRoot(x, 2) * nthRoot(x, 2))^2', 'x^2'], | ||
| // TODO: fix nthRoot to evaluate nthRoot(x^3, 2) | ||
| // ['(nthRoot(x, 2))^3', 'nthRoot(x ^ 3, 2)'], | ||
| // ['3 * nthRoot(x, 2) * (nthRoot(x, 2))^2', '3 * nthRoot(x ^ 3, 2)'], |
3 participants
Fix the issue where
(nthRoot(x,2))^2does not evaluate to x (returns no steps).