Enhancement: Multiply denominators with terms #88

arbylee · 2017-01-22T11:44:16Z

Attempt to resolve #51 and implement removeSymbolFromDenominator.
This change also needed to enhance the cancelLikeTerms logic. Cancelling needed to account for denominators matching what was being multiplied. Otherwise if they didn't cancel out, it would continue trying to multiply the denominator across until the equation reaches the length limit. Let me know any feedback

evykassirer · 2017-01-22T22:18:21Z

Looking sweet! Thanks so much for this big change and also all the little changes you're making to make the codebase cleaner 🌟

I'm feeling a bit sick today, so I'll take a more thorough look early this week. I also hope to chat about this a bit with Ahmed, who wrote the original equations code.

evykassirer

Thank you!!

I made a whole bunch of comments:

a bunch are gratitude for helpful things you did 😄
a bunch are also formatting/documentation related
some are small changes for you to make

For the most part I think this approach is right. My one concern is around the additions to cancelling out- the is existing code is already is a bit more messy than average, so I'm happy to discuss what we can do a bit more once you've read over the comments.

You're the first contributor to make a change this big - thanks so much for your time, and for your patience in waiting for me to have the time to review it ;D

I'm also in general really excited for this change because we'll be able to solve a whole bunch more equations thanks to you!

evykassirer · 2017-01-22T22:06:04Z

lib/Symbols.js

@@ -28,7 +28,7 @@ Symbols.getLastSymbolTerm = function(node, symbolName) {
  if(isSymbolTerm(node, symbolName)) {
    return node;
  }
-  // Otherwise, it's a sum of terms. Look through the operands for a term
+  // If it's a sum of terms, look through the operands for a term


evykassirer · 2017-01-22T22:08:57Z

lib/Symbols.js

+      return new Node.PolynomialTerm(node).getCoeffNode();
+    } catch (e) {
+      if (e.message.startsWith('denominator must be constant node')) {
+        // Not sure what this should return for a case such as 1/(2x)


probably just return null like below - unless you want to use part of the 1/(2x) term somehow

and even if the error isn't "denominator must be constant node..." you would also want to return null right?

we should probably better define what "symbol term" and "non symbol term" mean for things like 1/2x, depending on whatever's useful for solving equations -- and then yeah I think moving this stuff to an equation specific file would make more sense

evykassirer · 2017-01-22T22:12:00Z

lib/Symbols.js

 // Returns if `node` is a polynomial term with symbol `symbolName`
 function isSymbolTerm(node, symbolName) {
  if (Node.PolynomialTerm.isPolynomialTerm(node)) {
    const polyTerm = new Node.PolynomialTerm(node);
    if (polyTerm.getSymbolName() === symbolName) {
      return true;
    }
+  } else if (hasDenominatorSymbol(node, symbolName)) {
+    return true


missing semi colon! can you run the linter with npm lint to make sure you're not missing other stuff?

also, generally we put else on new line, so:

return true; } } else if (hasDenominatorSymbol(node, symbolName)) {

(I just added this to #44)

evykassirer · 2017-01-22T22:12:58Z

lib/Symbols.js

  }
  return false;
 }

+function hasDenominatorSymbol(node, symbolName) {
+  if (Node.Type.isOperator(node) && node.op === '/') {
+    return Symbols.getSymbolsInExpression(node).size > 0;


you should use symbolName here to make sure that the symbol in the expression is indeed the symbol we were looking for

evykassirer · 2017-01-22T22:13:22Z

lib/node/PolynomialTerm.js

@@ -158,6 +90,7 @@ PolynomialTerm.isPolynomialTerm = function(
    node, onlyImplicitMultiplication=false) {
  try {
    // will throw error if node isn't poly term
+    //


(extra line)

evykassirer · 2017-01-25T21:06:39Z

lib/node/PolynomialTerm.js

@@ -179,4 +112,88 @@ function negativeCoefficient(node) {
  return node;
 }

+function parseNode(node, onlyImplicitMultiplication) {


can you add a docstring, now that it's its own function?

evykassirer · 2017-01-25T21:08:05Z

lib/simplifyExpression/fractionsSearch/cancelLikeTerms.js

@@ -20,6 +20,52 @@ class CancelOutStatus {
 // e.g. (2x^2 * 5) / 2x^2 => 5 / 1
 // Returns a Node.Status object
 function cancelLikeTerms(node) {
+  // Cancel out terms if the term being multiplied
+  // is the same as the denominator
+  if (Node.Type.isOperator(node) && node.op === '*') {


can you merge in master (now with #91) and change this to isOperator(node, '*')? thanks!

evykassirer · 2017-01-25T21:28:42Z

lib/simplifyExpression/fractionsSearch/cancelLikeTerms.js

@@ -20,6 +20,52 @@ class CancelOutStatus {
 // e.g. (2x^2 * 5) / 2x^2 => 5 / 1
 // Returns a Node.Status object
 function cancelLikeTerms(node) {
+  // Cancel out terms if the term being multiplied


I think this comment isn't super clear about what 'the term being multiplied' is
can you add an example?

evykassirer · 2017-01-25T21:29:38Z

lib/simplifyExpression/fractionsSearch/cancelLikeTerms.js

+  if (Node.Type.isOperator(node) && node.op === '*') {
+    let newNode = clone(node);
+    let leftNode = newNode.args[0];
+    let rightNode = newNode.args[1];


what if the roles you gave left node and right node are reversed? e.g. the right node is a fraction or sum that cancels out with the left node?

also, how do you know there's only two things being multiplied by each other? there can be any number of terms in a multiplication

I'm actually surprised you need this - I think that multiplyFractions will turn (2+x) * (3+x)/(2+x) into ((2+x)(3+x))/(2+x) which should then just cancel out with the existing code... but if you had to add this stuff, there's probably something missing here that needs to be added. Not sure if what you have fills the hole quite right, but let's talk about it more!

The code on master doesn't cancel out terms for (2+x) * (3+x)/(2+x), which was what prompted these changes. Currently it would distribute out the (2+x). Without canceling, my changes caused an infinite loop by multiplying the denominator, distributing out, multiplying the denominator, etc.

Regarding the left node / right node positioning, I had made an assumption based on what would happen during equations steps, but you're right that it misses out on if an equation starts as (2+x) * (3+x)/(2+x). I'll work on resolving this in the next commit

From what I can tell that scenario wouldn't occur during equation solving though. Instead of multiplying by an inverted fraction, the steps would multiply by the denominator and then divide by the numerator

evykassirer · 2017-01-25T21:32:21Z

lib/simplifyExpression/fractionsSearch/cancelLikeTerms.js

+    // being multiplied
+    if (Node.Type.isOperator(leftNode) && leftNode.op === '+') {
+      let allMatchingDenominators = true;
+      for (let i = 0; i < leftNode.args.length; i++) {


you could use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/every here, I think

arbylee · 2017-01-29T02:35:54Z

I'm actually starting to think that this enhancement is a bit premature. There's a lot of functionality that I seem to need out of cancelLikeTerms that should probably be done first before working on the equation solving. For multiplication across, these things need to be handled first
(x+1)/(1-x) * (1-x) -> x + 1
(3/(1-x) + 3/(1+x)) * (1-x) -> 6-3x
((x+1)/(1-x) + 3/(1-x)) * (1-x) -> x + 4

I'm also not sure how the current approach would deal with factorization and canceling:
(x^2 + 4)/(x-2) -> x+2

I think I'll pull out some of the generic changes here out into a separate pull request, but I'm going to pause progress on this PR and maybe look into canceling terms separately when I have some time. I feel like that's going to be a considerable beast on its own

evykassirer · 2017-01-31T01:02:58Z

I honestly think we can merge a lot of the stuff in this PR :)

if in simplifyExpression/stepThrough.js you switch the order of multiplyFractionsSearch and breakUpNumeratorSearch, (x+1)/(1-x) * (1-x) would become [(x+1) * (1-x)] / (1-x) and then cancelling will work. This will then cover most of the simple cases - which is why I think it's worth making the change even though some other things won't work properly yet.

(3/(1-x) + 3/(1+x)) * (1-x) -> 6-3x and ((x+1)/(1-x) + 3/(1-x)) * (1-x) -> x + 4 could be fixed with a change to distribution where if (exactly) one of the blocks being distributed into each other has fractions, we take the other block and add it to the numerator (not sure if I'm describing this well enough, but I think that one case will fix that issue for the most part, as well as ((x+1)/(1-x) + 3/(1-x)) * (1-x))

(aside, (3/(1-x) + 3/(1+x)) * (1-x) -> 6/(x+1) but we'll have to wait for adding polynomial fractions and common denominators there and stuff to get that covered)

For factoring, we haven't done any factoring stuff yet but it's on the horizon! There's a lot of cases where your code would be helpful without needing factoring, and I think it's quite reasonable to add this functionality now and wait for factoring later.

tl;dr, I think we should:

switch the order of multiplyFractionsSearch and breakUpNumeratorSearch and remove your addition to the cancelling terms code
merge this change!
later, add distribution rules and factoring to handle some cancelling cases better

what do you think?

kevinbarabash · 2017-01-31T04:25:38Z

((x+1)/(1-x) + 3/(1-x)) * (1-x) -> x + 4 erases information about which values of x are invalid so the expressions are 100% equivalent. It's probably be worth keeping track of the fact that 1 - x can't be 0.

arbylee · 2017-01-31T10:06:08Z

Sure, I'll try flipping those methods tomorrow and see where I get

evykassirer · 2017-01-31T14:21:26Z

@kevinbarabash very true. We don't have anything that stores information like that yet. Do you think adding it to the NodeStatus object would be a good way to do it?

kevinbarabash · 2017-01-31T14:37:59Z

@evykassirer a new NodeStatus object is created for each step, right? If that's the cas we'd probably want to copy any solution constraints to all subsequent steps so that it's easy to check. Any new solution constraints would have to be unioned with previous ones. The concept of solution constraints could also be applied to things like nthRoot(x) and other functions in the future.

evykassirer · 2017-02-13T16:42:27Z

hey @arbylee - did you get a chance to try it? how'd it go? :)

arbylee · 2017-02-13T23:16:05Z

Hey @evykassirer, I did try removing the cancelLikeTerms changes and flipping multiplyFractionsSearch and breakUpNumeratorSearch. It ended up breaking a few test cases, and I haven't had enough time to look through them (I moved this past weekend). I've updated the branch if you want to have a look

evykassirer · 2017-02-13T23:22:12Z

thanks! I'll take a look and see what makes sense to fix here and what makes sense to fix in other issues

this change is a nice goldmine of revealing bugs ;D

evykassirer

ah, okay I see what you mean about not really being able to merge this until those other things are fixed... otherwise none of it works haha!

I opened issues for the bugs, for exactly what needs to be fixed. Feel free to take them on, or we can wait until someone else can do them and then come back to this.

I also added some changes you can make in the meantime.

Hope you're settling into your new place nicely!

evykassirer · 2017-02-22T04:18:05Z

lib/Symbols.js

@@ -49,7 +49,13 @@ Symbols.getLastSymbolTerm = function(node, symbolName) {
 // e.g. 4x^2 + 2x + y with `symbolName=x` would return y
 Symbols.getLastNonSymbolTerm = function(node, symbolName) {
  if (isSymbolTerm(node, symbolName)) {
-    return new Node.PolynomialTerm(node).getCoeffNode();
+    try {


actually this might be clearer to do a check for if it's a polynomial term instead of the throw catch - that's commonly used with the PolynomialTerm constructor actually

evykassirer · 2017-02-22T04:19:18Z

lib/Symbols.js

@@ -63,6 +69,30 @@ Symbols.getLastNonSymbolTerm = function(node, symbolName) {
  return null;
 };

+// Iterates through a node and returns the last term that has a
+// symbolName in its denominator


it actually returns the denominator, not the term, right?

Yes you're right

evykassirer · 2017-02-22T04:21:12Z

lib/Symbols.js

@@ -71,7 +101,17 @@ function isSymbolTerm(node, symbolName) {
      return true;
    }
  }
+  else if (hasDenominatorSymbol(node, symbolName)) {


why did you add this to isSymbolTerm?

It's been a while now so I'm trying to remember. I believe I had changed isSymbolTerm to return true if there was a symbol in the denominator, and there's logic that would then call getLastSymbolTerm. And then getLastSymbolTerm relies on this isSymbolTerm function

Jumping back to this because it should probably be discussed (and have code/comments updated appropriately). Prior to this change, isSymbolTerm specifically was checking only for polynomial symbol terms. So a term like 1/x would not be considered a symbol term because it's not a polynomial term. This change makes it so that terms with a symbol in the denominator are considered symbol terms as well.

In turn, this affects getLastNonSymbolTerm and getLastSymbolTerm which both use isSymbolTerm. I'm not sure if this change is going against the intentions of the Symbols object. If it is, then I'll need to find some other way to get a symbol term in a denominator

This makes sense to me! Agreed re: updating the comments to explain this

evykassirer · 2017-02-22T04:25:22Z

test/Symbols.test.js

+
+  tests.forEach(t => runTest(Symbols.getLastDenominatorSymbolTerm, t[0], t[1]));
+});
+


extra line here (you only need one new line at the bottom of a file)

evykassirer · 2017-02-22T04:25:29Z

test/Symbols.test.js

+  tests.forEach(t => runTest(Symbols.getLastSymbolTerm, t[0], t[1]));
+});
+
+describe('getLastDenominatorSymbolTerm', function() {


evykassirer · 2017-02-22T04:46:52Z

test/solveEquation/solveEquation.test.js

@@ -58,6 +58,12 @@ describe('solveEquation for =', function () {
    ['2(x+3)/3 = 2', 'x = 0'],
    ['( u )/( 0.3) = 4u + 6.28', 'u = -9.42'],
    ['- q - 4.36= ( 2.2q )/( 1.8)', 'q = -1.962'],
+    //['2/x = 1', 'x = 2'], TODO: fixme
+    //['2/(4x) = 1', 'x = 1/2'], TODO: bombs out


this is also related because (2 / (4x)) * 4x = 4x doesn't get cancelled out

I can see that we'll have to fix that first before we can actually test any of this (or have this functionality work at all)

evykassirer · 2017-02-22T04:57:38Z

test/solveEquation/solveEquation.test.js

@@ -58,6 +58,12 @@ describe('solveEquation for =', function () {
    ['2(x+3)/3 = 2', 'x = 0'],
    ['( u )/( 0.3) = 4u + 6.28', 'u = -9.42'],
    ['- q - 4.36= ( 2.2q )/( 1.8)', 'q = -1.962'],
+    //['2/x = 1', 'x = 2'], TODO: fixme
+    //['2/(4x) = 1', 'x = 1/2'], TODO: bombs out
+    //['2/(8 - 4x) = 1/2', 'x = 1'], TODO: fixme


same thing... :(

(2 / (8 - 4x)) * (8 - 4x) = (1/2) * (8 - 4x) 4 - 2x = (2 / (8 - 4x)) * 8 + (2 / (8 - 4x)) * -4x (4 - 2x) - 4 = ((2 / (8 - 4x)) * 8 + (2 / (8 - 4x)) * -4x) - 4 (-2x) / -2 = (((2 / (8 - 4x)) * 8 + (2 / (8 - 4x)) * -4x) - 4) / -2 ....

evykassirer · 2017-02-22T04:58:22Z

test/solveEquation/solveEquation.test.js

+    //['2/x = 1', 'x = 2'], TODO: fixme
+    //['2/(4x) = 1', 'x = 1/2'], TODO: bombs out
+    //['2/(8 - 4x) = 1/2', 'x = 1'], TODO: fixme
+    //['2/(1 + 1 + 4x) = 1/3', 'x = 1'], TODO: fixme


and again

(2 / (4x + 2)) * (4x + 2) = (1/3) * (4x + 2) 4x / 3 + 2/3 = (2 / (4x + 2)) * 4x + (2 / (4x + 2)) * 2 (4x / 3 + 2/3) * 3 = ((2 / (4x + 2)) * 4x + (2 / (4x + 2)) * 2) * 3

evykassirer · 2017-02-22T05:01:15Z

test/solveEquation/solveEquation.test.js

+    //['2/(4x) = 1', 'x = 1/2'], TODO: bombs out
+    //['2/(8 - 4x) = 1/2', 'x = 1'], TODO: fixme
+    //['2/(1 + 1 + 4x) = 1/3', 'x = 1'], TODO: fixme
+    //['(3 + x) / (x^2 + 3) = 1', '-x^2 + x = 0'], TODO: bombs out


this problem is different! it's a bad distribution

(3 / (x^2 + 3) + x / (x^2 + 3)) * (x^2 + 3) turns into
(3 / (x^2 + 3) * x^2 + 9 / (x^2 + 3) + x / (x^2 + 3) * x^2 + 3x / (x^2 + 3))

I think distribution should check to see if one of the multiplying things is also a list of fractions, and if only one of them is, then multiply the other one into its numerators. I'll make this a separate issue #138

eventually it'd be nice to not split the numerator like that, but I'm not sure when we'd want and when we wouldn't

evykassirer · 2017-02-22T05:16:19Z

test/solveEquation/solveEquation.test.js

+    //['2/(8 - 4x) = 1/2', 'x = 1'], TODO: fixme
+    //['2/(1 + 1 + 4x) = 1/3', 'x = 1'], TODO: fixme
+    //['(3 + x) / (x^2 + 3) = 1', '-x^2 + x = 0'], TODO: bombs out
+    //['6/x + 8/2x = 10', '(-10x) + 4x^2 = -6'], TODO: fixme


this is an interesting case where x * 6/x isn't multiplied to get (x * 6) / x, which would cancel out

one solution is just to have better cancelling code that doesn't need it to be a fraction.

another (easier for now) solution would be to add a case for a symbol times a symbol in the denominator (i.e. we don't want 9/2 * x to multiply to 9x / 2, but we would want 9/x * x to multiply to 9)

... anyways I'll open an issue #139 for this too

(btw 8/2x is parsed as 8/2 x ie 4x, not as 8 / 2x)

evykassirer · 2017-02-22T05:40:20Z

also @arbylee

do you want some mathsteps stickers as a thank you for your contributions? 😸

if you're interested, email mathsteps@socratic.org with your mailing address and I can send them over!

arbylee · 2017-02-24T12:34:33Z

@evykassirer Rebased and pushed with just the comment change for now. I'll have a look at some of the issues raised to see where I can help. And yes, I'd love some mathsteps stickers 😄

aelnaiem · 2017-03-30T16:40:48Z

Hi @arbylee, I'd love to get this into master, what can I do to support you?

evykassirer · 2017-03-30T17:01:35Z

@aelnaiem there are some things blocking this change from being useful, around cancelling

for this code to work, we first need to fix: #137 #138 and #139

(see conversation above for details on that)

aelnaiem · 2017-03-30T17:53:14Z

Great! So #138 is finished, and we only need #137 and #139 right? I'm thinking about how to denote high-priority issues. Those that block other things basically.

evykassirer · 2017-03-30T17:54:35Z

I think a 'high priority' tag would do the trick!

I'd also use it for things that are blocked but we want to do soon

ldworkin · 2017-07-11T14:36:22Z

So all blocking issues are taken care of now. @evykassirer how should we proceed? I could familiarize myself with this pull and try to fix the conflicts? Not sure if @arbylee is around to help?

evykassirer · 2017-07-11T15:48:14Z

If this is urgent for Socratic you could take it over right away, but if you have stuff to work on and can give @arbylee until next week to see this and finish if they want, I think that'd be ideal. Your call though, I know this was a pretty big thing

arbylee · 2017-07-18T04:24:04Z

I've rebased and I think addressed the remaining change requests on this. For the test case @ldworkin mentioned, I've only set it to NO_STEPS for now. I can't recall the context exactly (I think I just wanted to see how edge cases would behave in existing code), but I originally added that test in Cleanup from feedback, add comments, and rebase as NO_STEPS anyway. I think further simplification of that particular case could be handled as a separate issue. Thoughts?

ldworkin · 2017-07-18T11:58:22Z

Awesome, this looks great @arbylee. I'll make a separate note/issue to address that test case. @evykassirer thoughts on merging?!

evykassirer · 2017-07-19T02:22:40Z

kevin mentioned earlier

((x+1)/(1-x) + 3/(1-x)) * (1-x) -> x + 4 erases information about which values of x are invalid so the expressions are 100% equivalent. It's probably be worth keeping track of the fact that 1 - x can't be 0.

(see above for more context)

we should open an issue for this thing so we can address it once we make the NodeStatus more complicated and able to support constraints

evykassirer

ahhhh amazing thanks!!! just commented on a style nit, and one case I think you're missing, after that looks ready to go! So excited for this change :D

evykassirer · 2017-07-19T02:33:34Z

lib/Symbols.js

@@ -47,7 +47,9 @@ Symbols.getLastSymbolTerm = function(node, symbolName) {
 // e.g. 4x^2 + 2x + y with `symbolName=x` would return y
 Symbols.getLastNonSymbolTerm = function(node, symbolName) {
  if (isSymbolTerm(node, symbolName)) {
-    return new Node.PolynomialTerm(node).getCoeffNode();
+    if (Node.PolynomialTerm.isPolynomialTerm(node)) {


this would probably be cleaner as if (isSymbolTerm(node, symbolName) && Node.PolynomialTerm.isPolynomialTerm(node))

That would actually change the behavior of the function, but I've cleaned it up to be more obvious

evykassirer · 2017-07-19T03:11:58Z

lib/Symbols.js

+// symbolName in its denominator
+// e.g. 1/(2x) with `symbolName=x` would return 2x
+// e.g. 1/(x+2) with `symbolName=x` would return x+2
+// e.g. 1/(x+2) + (x+1)/(2x+3) with `symbolName=x` would return 2x+3


I'm curious - does it return 2x+3 or does it wrap it in parens like (2x+3) ?

It is wrapped in parens, updated

evykassirer · 2017-07-19T03:24:58Z

lib/Symbols.js

+// e.g. false for 5x
+function hasDenominatorSymbol(node, symbolName) {
+  if (Node.Type.isOperator(node) && node.op === '/') {
+    const allSymbols = Symbols.getSymbolsInExpression(node);


I think this will return true for x/2 too, so should this line be calling Symbols.getSymbolsInExpression only on the denominator?

You're right. I've corrected that now

evykassirer · 2017-07-19T03:33:34Z

lib/solveEquation/EquationOperations.js

-  // pass for now
+// Ensures that a symbol is not in the denominator by multiplying
+// both sides by the denominator if there is a symbol present.
+EquationOperations.removeSymbolFromDenominator = function(equation, symbolName) {


evykassirer · 2017-07-19T03:34:29Z

lib/solveEquation/EquationOperations.js

+  const leftNode = equation.leftNode;
+  const denominator = Symbols.getLastDenominatorWithSymbolTerm(leftNode, symbolName);
+  if (denominator) {
+    return performTermOperationOnEquation(


I love how simple this change is thanks to the great work @aliang8 and @ldworkin have done 😍

evykassirer · 2017-07-19T03:37:00Z

test/Symbols.test.js

+    ['x/(x+3)', 'x / (x + 3)'],
+  ];
+
+  tests.forEach(t => runTest(Symbols.getLastSymbolTerm, t[0], t[1]));


can you pass 'x' in too, and then also have some tests where there are multiple symbols and you are looking for one of them? e.g. '1/x + y', y

Added some test cases with other variables

evykassirer · 2017-07-19T03:38:04Z

test/Symbols.test.js

+    ['1/x', 'x'],
+    ['1/(x+2)', '(x + 2)'],
+    ['1/(x+2) + 3x', '(x + 2)'],
+    ['1/(x+2) + 3x/(1+x)', '(1 + x)'],


can you add 2 + 2/x + x/2 to test that case I raised above?

Done. There was even an existing issue with how symbols were being parsed that was made obvious with this particular test case. I've added a commit to recursively parse the nodes for symbols. It was previously only comparing 2 + 2/x and x/2 as the only two nodes, instead of checking 2 and 2/x individually

yay! glad we caught that :D

evykassirer · 2017-07-19T03:38:35Z

test/solveEquation/solveEquation.test.js

+    ['2/(8 - 4x) = 1/2', 'x = 1'],
+    ['2/(1 + 1 + 4x) = 1/3', 'x = 1'],
+    ['(3 + x) / (x^2 + 3) = 1', 'x = [0, 1]'],
+    ['6/x + 8/2x = 10', NO_STEPS],


can you add a TODO for this just so it's clear that this isn't what we want?

I realized that the test I wanted from this was actually missing parens in 8/(2x). I've added that and adjusted the test result.

I didn't leave the previous test case without parens because it started to look similar to the test cases in the other TODO related to complex numbers. I can add it back with a comment if you think it's a separate scenario

@ldworkin how do you want to track this? You said "I'll make a separate note/issue to address that test case."

is it in your Asana board? or can we open an issue here? should we just put the TODO in the code? I'm fine as long as it's being noted somewhere :) so we remember

It's in our Asana

ldworkin · 2017-07-19T12:31:27Z

So excited for this, thanks for all your work, @arbylee !!

ldworkin · 2017-07-19T12:57:11Z

Dammit, this change a21240d broke something! Investigating, will try to fix.

ldworkin · 2017-07-19T13:27:43Z

Ok, I ended up reverting that change. I need to revisit the logic there. So this should be good -- not sure why it says there are still conflicts/failing checks, I think if we merge the latest master into it, it will be all good.

This reverts commit 2a35214.

These functions don't necessarily return just polynomial terms anymore since isSymbol now returns true if there is a term in the denominator such as 1/x, which is not a polynomial term

The parsed nodes were previously comparing something like: 2 + 2/x + x/2 as 2 nodes. One node of `2 + 2/x` being added to `x/2` We needed to drill down to check `2` and `2/x` and `x/2` as individual nodes

evykassirer · 2017-07-19T18:49:47Z

lib/Symbols.js

@@ -76,10 +83,13 @@ Symbols.getLastDenominatorWithSymbolTerm = function(node, symbolName) {
  // Otherwise, it's a sum of terms. e.g. 1/x + 1(2+x)
  // Look through the operands for a
  // denominator term with `symbolName`
-  else if (Node.Type.isOperator(node) && node.op === '+') {
+  else if (Node.Type.isOperator(node, '+')) {


evykassirer

🎉 so excited about this, thanks @arbylee

evykassirer mentioned this pull request Jan 25, 2017

Missing test coverage #58

Open

3 tasks

evykassirer suggested changes Jan 25, 2017

View reviewed changes

arbylee force-pushed the remove-denominators branch from e0274dd to 8eb739b Compare January 28, 2017 23:59

kevinbarabash mentioned this pull request Jan 31, 2017

Keep track of solution constraints #114

Open

aelnaiem mentioned this pull request Feb 1, 2017

Create a list of goal test cases #96

Closed

This was referenced Feb 22, 2017

multiply fractions with parenthesis #137

Closed

handle distribution with fractions more smartly #138

Closed

multiply fractions edge case to support cancelling #139

Closed

evykassirer reviewed Feb 22, 2017

View reviewed changes

arbylee force-pushed the remove-denominators branch from 86505eb to b8a625d Compare February 24, 2017 12:30

arbylee force-pushed the remove-denominators branch from b8a625d to b66688c Compare February 24, 2017 12:39

evykassirer added the blocked label Mar 30, 2017

evykassirer removed the blocked label Jul 19, 2017

evykassirer reviewed Jul 19, 2017

View reviewed changes

arbylee force-pushed the remove-denominators branch from bc08502 to 845f5b1 Compare July 19, 2017 07:26

arbylee and others added 17 commits July 19, 2017 08:57

Add Parenthesis Node to PolynomialTerm constructor

d70a035

Implement EquationOperations.removeSymbolFromDenominator

2b0a6c4

Cleanup from feedback, add comments, and rebase

8943c32

Remove cancelLikeTerms changes and reorder simplify expressions

187d3c7

Comment out broken tests

8fe8ac8

Update test comment

4d106a4

Uncomment passing test cases

72b33a3

Resolve linting errors

e653802

Remove a try/catch and function renaming

d01fb36

Update PolynomialTerm.js

cf259d0

Revert "Update PolynomialTerm.js"

e3cbb16

This reverts commit 2a35214.

Update hasDenominatorSymbol with comment and explicit boolean return

b71378c

Update comments about polynomials in Symbols

8df109f

These functions don't necessarily return just polynomial terms anymore since isSymbol now returns true if there is a term in the denominator such as 1/x, which is not a polynomial term

Fix hasDenominatorSymbol to only check denominator for symbols

a3fa2a0

Add missing parens in test case

5de94ae

Recursively check symbol nodes until you have a term without addition

16db38a

The parsed nodes were previously comparing something like: 2 + 2/x + x/2 as 2 nodes. One node of `2 + 2/x` being added to `x/2` We needed to drill down to check `2` and `2/x` and `x/2` as individual nodes

Clean up getLastNonSymbolTerm

1708900

arbylee force-pushed the remove-denominators branch from 42a0604 to 1708900 Compare July 19, 2017 15:58

evykassirer reviewed Jul 19, 2017

View reviewed changes

evykassirer approved these changes Jul 19, 2017

View reviewed changes

evykassirer merged commit 1cbae0f into google:master Jul 19, 2017


		tests.forEach(t => runTest(Symbols.getLastDenominatorSymbolTerm, t[0], t[1]));
		});

Enhancement: Multiply denominators with terms #88

Enhancement: Multiply denominators with terms #88

Conversation

arbylee commented Jan 22, 2017

evykassirer commented Jan 22, 2017

evykassirer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbylee commented Jan 29, 2017

evykassirer commented Jan 31, 2017

kevinbarabash commented Jan 31, 2017

arbylee commented Jan 31, 2017

evykassirer commented Jan 31, 2017

kevinbarabash commented Jan 31, 2017

evykassirer commented Feb 13, 2017

arbylee commented Feb 13, 2017

evykassirer commented Feb 13, 2017

evykassirer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer commented Feb 22, 2017

arbylee commented Feb 24, 2017

aelnaiem commented Mar 30, 2017

evykassirer commented Mar 30, 2017 • edited Loading

aelnaiem commented Mar 30, 2017

evykassirer commented Mar 30, 2017

ldworkin commented Jul 11, 2017

evykassirer commented Jul 11, 2017

arbylee commented Jul 18, 2017

ldworkin commented Jul 18, 2017

evykassirer commented Jul 19, 2017

evykassirer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldworkin commented Jul 19, 2017

ldworkin commented Jul 19, 2017

ldworkin commented Jul 19, 2017

Choose a reason for hiding this comment

evykassirer commented Mar 30, 2017 •

edited

Loading

evykassirer Jul 19, 2017 •

edited

Loading