#163 - Better sum-product factoring steps #210

alainakafkes · 2017-07-21T15:51:08Z

Following the conventions used in addConstantFractions.js, I broke down factorQuadratic into 3 substeps:

Break apart the middle term into two terms using our factor pair (p and q): ax^2 + bx + c --> ax^2 + px + qx + c
Factor both groups separately: first group as [ux(rx + s)] and second group as [v(rx + s)]
Finish factoring by combining the factored terms through grouping: (ux + v)(rx + s)

Please let me know if there are any problems and how I can fix them! 😊

evykassirer

This looks great! Just left a few small comments

I'd love to see some new tests showing how this works - there are some tests already that test substeps (the fractions code you were basing this off of probably has some!) so you can do some stuff similar to that!

lemme know if you have any questions ^_^

evykassirer · 2017-07-22T00:44:42Z

lib/factor/factorQuadratic.js

-        // TODO: these should be actual substeps
-        // 1. Break apart the middle term into two terms using our factor pair (p and q):
-        //    e.g. ax^2 + bx + c -> ax^2 + px + qx + c
+        // TODO: these should be actual substeps *alaina*


you can get rid of this TODO now that it's done 🎉 :D

evykassirer · 2017-07-22T23:27:05Z

lib/factor/factorQuadratic.js

+        let status;
+
+        // 1. Break apart the middle term into two terms using our factor pair
+        // (p and q)


I think it's a bit redundant to put the steps above and also here - I could see putting them in detail at the beginning (so you can see them all together) and then just // STEP 1 above step 1 for example - but putting each detailed comment above each step is also fine.

The way you have it here probably is also fine (yay comments) so feel to disagree

evykassirer · 2017-07-22T23:27:12Z

lib/factor/factorQuadratic.js

        const pValue = pair[0];
        const qValue = pair[1];

+        // SUBSTEP: ax^2 + bx + c -> ax^2 + px + qx + c
+        const sq = Node.Creator.constant(2);


sq was confusing for me as I read it over, at first - maybe define it right before you create the node that uses it, or rename it to two or just put it right in the other creator function Node.Creator.polynomialTerm(symbol, Node.Creator.constant(2), a); or some combination of those ^_^

evykassirer · 2017-07-22T23:27:27Z

lib/factor/factorQuadratic.js

+        const qx = Node.Creator.polynomialTerm(symbol, null, q);
+        newNode = Node.Creator.operator('+', [ax2, px, qx, c], true);
+        status = Node.Status.nodeChanged(
+          ChangeTypes.SIMPLIFY_ARITHMETIC, node, newNode);


I'd make a new change type for this since it's a pretty unique use case and it'd be nice to describe it more accurately :)

evykassirer · 2017-07-22T23:27:31Z

lib/factor/factorQuadratic.js

+        substeps.push(status);
+        newNode = Node.Status.resetChangeGroups(status.newNode);
+
+        // 2. Consider the first two terms together and the second two terms


I find it odd how we point this out in the code but not in the steps we show. I think we should either wrap them in parens (ax^2 + px) + (qx + c) and make it a step, or take out references to it in the code and change 'Factor the first grouptoFactor the first two terms` or something

Not your fault - you did what it said - just leftover thinking about Ahmed's planning as we move into concrete substeps ^_^

evykassirer · 2017-07-22T23:27:37Z

lib/factor/factorQuadratic.js

@@ -249,6 +276,7 @@ function factorSumProductRule(node, symbol, aValue, bValue, cValue, negate) {
        const firstParen = Node.Creator.parenthesis(
          Node.Creator.operator('+', [rx, s]));

+        // 3B. Factor the second group


I'm wondering if it'd be better to factor the first group and second group in two different substeps instead of the same one - what do you think?

Agree! I'm going to implement this.

evykassirer · 2017-07-22T23:27:40Z

lib/factor/factorQuadratic.js

@@ -262,8 +290,17 @@ function factorSumProductRule(node, symbol, aValue, bValue, cValue, negate) {
        const secondParen = Node.Creator.parenthesis(
          Node.Creator.operator('+', [ux, v]));

+        // SUBSTEP: ux(rx + s) + v(rx + s)


can you do a before -> after here too like you did above? :D

(and add it below too)

evykassirer · 2017-07-22T23:29:33Z

lib/factor/factorQuadratic.js

        const pValue = pair[0];
        const qValue = pair[1];

+        // SUBSTEP: ax^2 + bx + c -> ax^2 + px + qx + c
+        const sq = Node.Creator.constant(2);
+        const a = Node.Creator.constant(aValue);


I think there are some edge cases to consider here - the only one I can think of off the top of my head is that 'a' could defs be 1 here (i.e. x^2 + bx + c) - maybe poke around a bit in the logic that calls this code to see what is for sure defined and what isn't? I'll think a bit more about this too

Ah, I'm a bit confused by this. What would be the problem when a = 1?

oh I think in that case you'd end up with a step like 1x^2 + 2x + 3x + 6 or something, where the 1 shouldn't actually be there

@alainakafkes ^

@evykassirer I think based on the tests I wrote, that extra "1" coefficient doesn't show up in front of x^2. Let me know if I've misunderstood!

oh cool that must be handled somewhere else in the code then!

yay tests and yay code that just does what it should 🎉

evykassirer · 2017-07-26T20:42:18Z

lib/ChangeTypes.js


  // e.g. 2 + x + 3 + x -> 5 + 2x
  COLLECT_AND_COMBINE_LIKE_TERMS: 'COLLECT_AND_COMBINE_LIKE_TERMS',
  // e.g. x + 2 + x^2 + x + 4 -> x^2 + (x + x) + (4 + 2)
  COLLECT_LIKE_TERMS: 'COLLECT_LIKE_TERMS',
+  // e.g. 2x^2 + 4x + 2 -> 2x^2 + 2x + 2x + 2
+  BREAK_UP_TERM: 'BREAK_UP_TERM',


might make more sense to put this rule where the factoring changegroups are, since that helps give context into what it's used for

evykassirer · 2017-07-27T00:44:48Z

lib/factor/factorQuadratic.js

+        const px = Node.Creator.polynomialTerm(symbol, null, p);
+        const qx = Node.Creator.polynomialTerm(symbol, null, q);
+
+        // SUBSTEP: ax^2 + bx + c -> ax^2 + px + qx + c


mrah sorry for not commenting on this last time, but I think it's confusing to say substep after step - I think just merge them into one comment at the top

// SUBSTEP 1: ax^2 + bx + c -> ax^2 + px + qx + c ... // SUBSTEP 2 ...

does that make sense?

Yes, I can see that... Oops 😱

aelnaiem · 2017-08-03T01:04:11Z

Dope!

alainakafkes · 2017-08-03T11:23:53Z

Okay @evykassirer I think I addressed all of your requested changes! 🎉 Please let me know if I should make any more.

evykassirer · 2017-08-06T01:21:40Z

changes look great! just one unaddressed comment I commented on

also

I'd love to see some new tests showing how this works - there are some tests already that test substeps (the fractions code you were basing this off of probably has some!) so you can do some stuff similar to that!

evykassirer · 2017-08-09T17:42:54Z

test/factor/factorQuadratic.test.js

+        '(x - 1) * (x + 2)']
+    ],
+    ['-x^2 - 3x - 2',
+      ['x^2 + x + 2x + 2', // per factorQuadratic, negative is added back in at the end


oo good test

this made sense when we didn't have substeps - here can we wrap all the substeps in unary minus?

the substeps should probably look like:

-x^2 - 3x - 2 - (x^2 + 3x + 2) - ((x^2 + x) + (2x + 2)) - (x * (x + 1) + (2x + 2)) .... - (x * (x + 1) + 2 * (x + 1)) - -(x + 1) * (x + 2)

what do you think? sorry I keep asking you to do more things hehe

Lol it's ok! So I wasn't sure what to do here when I wrote this test. I knew that the negative sign would be added back at the end, but I didn't know how to convey that in the test case.

When I tried what you just suggested, I got this error from deepEqual. Should I leave what I had or does this point to a deeper problem with deepEqual?

mm so I think the unary minus just gets dropped in the code right now (unless I'm missing something) so you'd wanna go in and update the code when making the steps to add the unary minus bit around each substep if needed

edge casssesssss

Yes that makes much more sense. 😂 Looks I just had to negate each newNode along the way.

sweet - not too bad of a fix then whew

evykassirer · 2017-08-09T17:43:11Z

test/factor/factorQuadratic.test.js

+        '(x^2 + x) + (2x + 2)',
+        'x * (x + 1) + (2x + 2)',
+        'x * (x + 1) + 2 * (x + 1)',
+        '(x + 1) * (x + 2)']


eee this is awesome look at this sum product rule awesomeness

such a tiny nit I didn't want to ask you to do it :p

evykassirer

YAY this is awesome thanks @alainakafkes :D

alainakafkes added 3 commits July 19, 2017 15:40

Creates three substeps for factorSumProductRule

2496462

Fixes casing for all Node.Status.nodeChanged()

e2495a2

Removes unused const b

3704369

evykassirer suggested changes Jul 22, 2017

View reviewed changes

Adds substeps 2, 3A, and 3B in factorQuadratic

aee480f

evykassirer reviewed Jul 26, 2017

View reviewed changes

evykassirer reviewed Jul 27, 2017

View reviewed changes

Fixes substep comments & moves BREAK_UP_TERM to factoring changegroups

10a4e9e

Adds tests for factorSumProductRule substeps

551d5dc

evykassirer reviewed Aug 9, 2017

View reviewed changes

alainakafkes and others added 4 commits August 9, 2017 14:30

Fixes unary minus edge case for factorSumProductRule tests

2470634

Adds optional substep for unary minus edge cases in factorSumProductRule

5328883

clarify comment

b2b06ac

such a tiny nit I didn't want to ask you to do it :p

oops line length

ffc3098

evykassirer approved these changes Aug 9, 2017

View reviewed changes

evykassirer merged commit c7d1f48 into google:master Aug 9, 2017

evykassirer mentioned this pull request Aug 9, 2017

Better sum-product factoring steps #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#163 - Better sum-product factoring steps #210

#163 - Better sum-product factoring steps #210

alainakafkes commented Jul 21, 2017

evykassirer left a comment

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

alainakafkes Jul 23, 2017

evykassirer Jul 22, 2017

evykassirer Jul 22, 2017

alainakafkes Jul 23, 2017

evykassirer Jul 26, 2017

evykassirer Aug 6, 2017

alainakafkes Aug 9, 2017

evykassirer Aug 9, 2017

evykassirer Jul 26, 2017

evykassirer Jul 27, 2017 •

edited

alainakafkes Aug 3, 2017

aelnaiem commented Aug 3, 2017

alainakafkes commented Aug 3, 2017

evykassirer commented Aug 6, 2017

evykassirer Aug 9, 2017

alainakafkes Aug 9, 2017

evykassirer Aug 9, 2017

evykassirer Aug 9, 2017

alainakafkes Aug 9, 2017

evykassirer Aug 9, 2017

evykassirer Aug 9, 2017

evykassirer left a comment

#163 - Better sum-product factoring steps #210

#163 - Better sum-product factoring steps #210

Conversation

alainakafkes commented Jul 21, 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 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelnaiem commented Aug 3, 2017

alainakafkes commented Aug 3, 2017

evykassirer commented Aug 6, 2017

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 left a comment

Choose a reason for hiding this comment

evykassirer Jul 27, 2017 •

edited