adding factoring support #104

aelnaiem · 2017-01-27T18:32:14Z

Issue: #50

Adds support for factoring by:

difference of squares
perfect squares
sum product rule

Quadratic equation to be added in the future. The reason I didn't add it yet is because it requires simplification of only what will be in the brackets (without expansion), and I didn't want to make a decision on how that should happen.

evykassirer

yay factoring! added some thoughts :)

evykassirer · 2017-02-04T22:11:04Z

lib/checks/isQuadratic.js

+
+const Node = require('../node');
+
+// TODO(ael)


evykassirer · 2017-02-04T22:11:40Z

lib/checks/isQuadratic.js

@@ -0,0 +1,45 @@
+'use strict';


can you merge in the new linter changes? apparently we don't need 'use strict' in modules because they're strict by default (the linter will catch this now 😄 )

evykassirer · 2017-02-04T22:12:22Z

lib/checks/isQuadratic.js

+
+// TODO(ael)
+function isQuadratic(node) {
+  if (!Node.Type.isOperator(node) || node.op !== '+') {


you can now do if (!Node.Type.isOperator(node, '+') thanks to a recent change :)

evykassirer · 2017-02-04T22:14:51Z

lib/checks/isQuadratic.js

+  const constantTerms = node.args.filter(Node.Type.isConstant);
+
+  if (secondDegreeTerms.length !== 1 || firstDegreeTerms.length > 1 ||
+    constantTerms.length !== 1) {


hmm why'd you choose this combination?

I agree that there should be a secondDegreeTerm for sure

but what about x^2 + x? that seems like a quadratic, but there's no constant term

Hmmmmmm. Interesting. You're right :)

evykassirer · 2017-02-04T22:15:26Z

lib/checks/isQuadratic.js

+
+  const secondDegreeTerms = node.args.filter(isSecondDegree);
+  const firstDegreeTerms = node.args.filter(isFirstDegree);
+  const constantTerms = node.args.filter(Node.Type.isConstant);


what if there are other args that don't fall into one of these buckets? we should return false right? so I guess check that the length of each of these add up to the number of args

evykassirer · 2017-02-04T22:41:43Z

lib/factor/factorQuadratic.js

+    if (constantTermValue < 0 &&
+        firstTermCoeffRootValue % 1 === 0 &&
+        constantTermRootValue % 1 === 0) {
+      const polyTerm = Node.Creator.polynomialTerm(symbol, null, firstTermCoeffRootNode);


can you split this line into 2 lines? (and everywhere else you have a line like this)

evykassirer · 2017-02-04T22:47:17Z

lib/factor/factorQuadratic.js

+    const secondTermCoeffValue = secondTermNode.getCoeffValue();
+    if (secondTermCoeffValue < 0) {
+      constantTermRootValue = constantTermRootValue * -1;
+      constantTermRootNode = Negative.negate(constantTermRootNode);


alternatively, you can just make constantTermRootNode at the times you need it instead of at the beginning

not sure if that's better or not

I have a slight preference for this because it becomes more obvious why you need it

evykassirer · 2017-02-04T22:50:03Z

test/factor/factorQuadratic.test.js

+    ['x^2 + 4x + 1', 'x^2 + 4x + 1'],
+  ];
+  tests.forEach(t => testFactorQuadratic(t[0], t[1]));
+});


can you add some tests for things that shouldn't factor yet? to make sure we're not treating things as difference of square when they're not, etc.

I'm especially cautious about stuff with signs e.g. -x^2 - 2x - 1

evykassirer · 2017-02-04T22:52:55Z

lib/factor/factorQuadratic.js

+      constantTermRootNode = Negative.negate(constantTermRootNode);
+    }
+
+    const perfectProduct = 2 * firstTermCoeffRootValue * constantTermRootValue;


some more comments describing how these variables fit into things might be helpful

I understood what was going on, but had to read over the code a couple times to get it

evykassirer · 2017-02-04T22:59:23Z

lib/factor/factorQuadratic.js

+      const factorPairs = ConstantFactors.getFactorPairs(constantTermValue, true);
+      for (const pair of factorPairs) {
+        if (pair[0] + pair[1] === secondTermCoeffValue) {
+          const polyTerm = Node.Creator.polynomialTerm(symbol, null, firstTermCoeffRootNode);


we know that the coeff is 1, so the polyTerm is just the symbol right?

evykassirer · 2017-02-07T02:15:05Z

lib/checks/isQuadratic.js

    return false;
  }

  if (node.args.length > 3) {
    return false;
  }

-  const secondDegreeTerms = node.args.filter(isSecondDegree);
-  const firstDegreeTerms = node.args.filter(isFirstDegree);
+  const symbolSet = Symbols.getSymbolsInExpression(node);


I think this might be confusing to an outsider - add a comment explaining what this is checking?

evykassirer · 2017-02-07T02:15:38Z

lib/checks/isQuadratic.js

-    return exponent && exponent.value === '2';
-  }
-  return false;
+function isPolynomialTermOfDegree(degree) {


add a docstring that also makes it clear we have to pass a string as the degree (which is unfortunate :p)

evykassirer · 2017-02-07T02:16:43Z

lib/checks/isQuadratic.js

-    const exponent = polyTerm.getExponentNode(true);
-    return exponent && exponent.value === '1';
+  // check that there are no terms that don't fall into these groups
+  if (secondDegreeTerms.length + firstDegreeTerms.length + constantTerms.length !== node.args.length) {


line is too long, idk how strict we wanna be about this though

evykassirer · 2017-02-07T02:19:16Z

lib/factor/factorQuadratic.js

@@ -21,19 +20,20 @@ function factorQuadratic(node) {
  }

  // get a, b and c
-  let firstTermNode, secondTermNode, constantTermValue;
+  let symbol, aValue, bValue, cValue;


maybe have them default to 0? what happens if they don't get set (if those terms aren't there)

evykassirer · 2017-02-07T02:20:29Z

lib/factor/factorQuadratic.js

+  }
+
+  // factor sum product rule e.g. x^2 + 3x + 2
+  status = factorSumProductRule(node, symbol, aValue, bValue, cValue, negate);


you could loop through these like we do in the other places where we check lots of different simplification functions

evykassirer · 2017-02-07T16:17:32Z

lib/factor/factorQuadratic.js

+        const aRootNode = Node.Creator.constant(aRootValue);
+
+        const polyTerm = Node.Creator.polynomialTerm(
+          symbol, null, aRootNode);


we know that aRootNode is 1 - so you could just not pass aRootNode at all right?

Node.Creator.polynomialTerm(symbol)

evykassirer · 2017-02-07T17:01:42Z

lib/simplifyExpression/collectAndCombineSearch/addLikeTerms.js

@@ -120,6 +121,7 @@ function addNegativeOneCoefficient(node) {
        polyTerm.getSymbolNode(),
        polyTerm.getExponentNode(),
        polyTerm.getCoeffNode(),
+        false,


add a comment for what this means maybe

evykassirer · 2017-02-07T17:08:00Z

lib/simplifyExpression/collectAndCombineSearch/addLikeTerms.js

@@ -84,7 +84,8 @@ function addPositiveOneCoefficient(node) {
      newNode.args[i] = Node.Creator.polynomialTerm(
        polyTerm.getSymbolNode(),
        polyTerm.getExponentNode(),
-        Node.Creator.constant(1));
+        Node.Creator.constant(1),
+        true);


maybe add comment for what true means

evykassirer · 2017-02-07T17:08:07Z

lib/factor/factorQuadratic.js

  return Node.Status.noChange(node);
 }

+


evykassirer · 2017-02-07T17:20:06Z

lib/node/Creator.js

@@ -48,12 +48,12 @@ const NodeCreator = {
  // exponent might be null, which means there's no exponent node.
  // similarly, coefficient might be null, which means there's no coefficient
  // the symbol node can never be null.
-  polynomialTerm (symbol, exponent, coeff, coeffIsNegOne=false) {
+  polynomialTerm (symbol, exponent, coeff, coeffIsOne=false, coeffIsNegOne=false) {


I wonder if we could combine coeffIsOne and coeffIsNegOne to a shared variable... something about explicit coefficients

or is that more confusing?

evykassirer · 2017-02-09T16:35:55Z

lib/checks/isQuadratic.js

@@ -12,13 +12,14 @@ function isQuadratic(node) {
    return false;
  }

+  // get the number of symbols in the expression and ensure there's only one


sorry I'm being really picky now so you don't have to change this but

I think this comment is pretty close to what the code is doing, I meant more like
"make sure only one symbol appears in the expression, i.e. just x and not both x and y"

the point of confusion for people could be that it's a set (which you show in the var name which is helpful, so it's probably fine) and there can be multiple symbol terms (multiple symbols) as long as they're of the same symbol name

evykassirer · 2017-02-09T16:46:43Z

lib/factor/factorQuadratic.js

-
-        const polyTerm = Node.Creator.polynomialTerm(
-          symbol, null, aRootNode);
+        const polyTerm = Node.Creator.polynomialTerm(symbol, null);


oh wait you can just use Node.Creator.symbol

evykassirer · 2017-02-09T16:48:31Z

I'm worried that 2x^2 + 2x -> 2x(x + 1) might still not work

evykassirer · 2017-02-09T16:55:29Z

lib/factor/factorQuadratic.js

+    const exponent = Node.Creator.constant(2);
+
+    if (negate) {
+      paren = Negative.negate(paren);


if you negate before doing the ^2, won't the negating cancel out?

ie (-(x+2))^2

I see your test passes though, which confuses me

evykassirer · 2017-02-09T17:00:51Z

still missing tests for factorSymbol with negate - I think there might be bugs there

evykassirer · 2017-02-09T17:04:39Z

lib/factor/factorQuadratic.js

@@ -56,35 +69,13 @@ function factorQuadratic(node) {
    cValue = -cValue;


these should default to 0 above, or else negating doesn't really make sense in the null case

evykassirer · 2017-02-09T17:06:04Z

added 6 comments, mostly just stuff you missed from the last code review

lemme know if there's anything I can do to make it easier for you to see and check off my comments to make sure you address them all (I know github doesn't really do a good job of making it easy)

…gative and also fixing negation when factoring

evykassirer · 2017-02-10T19:38:32Z

👍

adding factoring support

d9633f4

aelnaiem assigned evykassirer Jan 27, 2017

removing extra line

988b98e

aelnaiem mentioned this pull request Feb 1, 2017

Create a list of goal test cases #96

Closed

evykassirer suggested changes Feb 4, 2017

View reviewed changes

aelnaiem added 3 commits February 6, 2017 19:33

Merge branch 'master' into ae_quadratic

a527552

more tests, more functionality and code cleanup for factoring quadratics

93adba6

linting

d71c753

evykassirer suggested changes Feb 7, 2017

View reviewed changes

aelnaiem added 2 commits February 8, 2017 10:19

looping through factoring functions and adding more tests

f7ee0de

organizing change types by the type of change they are associated with

ce97fc3

evykassirer reviewed Feb 9, 2017

View reviewed changes

factoring out the gcd when factoring a symbol and adding tests for ne…

915f539

…gative and also fixing negation when factoring

aelnaiem merged commit 906cddf into master Feb 10, 2017

aelnaiem deleted the ae_quadratic branch February 10, 2017 17:22

evykassirer mentioned this pull request Feb 12, 2017

Expand "collect and combine like terms" to be more general (addition) #46

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding factoring support #104

adding factoring support #104

aelnaiem commented Jan 27, 2017 •

edited

evykassirer left a comment

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

aelnaiem Feb 6, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 4, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 7, 2017

evykassirer Feb 9, 2017

evykassirer Feb 9, 2017

evykassirer commented Feb 9, 2017

evykassirer Feb 9, 2017

evykassirer commented Feb 9, 2017

evykassirer Feb 9, 2017

evykassirer commented Feb 9, 2017

evykassirer commented Feb 10, 2017

		@@ -56,35 +69,13 @@ function factorQuadratic(node) {
		cValue = -cValue;

adding factoring support #104

adding factoring support #104

Conversation

aelnaiem commented Jan 27, 2017 • edited

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

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 9, 2017

Choose a reason for hiding this comment

evykassirer commented Feb 9, 2017

Choose a reason for hiding this comment

evykassirer commented Feb 9, 2017

evykassirer commented Feb 10, 2017

aelnaiem commented Jan 27, 2017 •

edited