Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding factoring support #104

Merged
merged 8 commits into from
Feb 10, 2017
Merged
2 changes: 2 additions & 0 deletions lib/ChangeTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ module.exports = {

// Factoring change types:

// e.g. x^2 - 4x -> x(x - 4)
FACTOR_SYMBOL: 'FACTOR_SYMBOL',
// e.g. x^2 - 4 -> (x - 2)(x + 2)
FACTOR_DIFFERENCE_OF_SQUARES: 'FACTOR_DIFFERENCE_OF_SQUARES',
// e.g. x^2 + 2x + 1 -> (x + 1)^2
Expand Down
51 changes: 28 additions & 23 deletions lib/checks/isQuadratic.js
Original file line number Diff line number Diff line change
@@ -1,45 +1,50 @@
'use strict';

const Node = require('../node');
const Symbols = require('../Symbols');

// TODO(ael)
// Given a node, will determine if the expression is in the form of a quadratic
// e.g. `x^2 + 2x + 1` OR `x^2 - 1` but not `x^3 + x^2 + x + 1`
function isQuadratic(node) {
if (!Node.Type.isOperator(node) || node.op !== '+') {
if (!Node.Type.isOperator(node, '+')) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (symbolSet.size !== 1) {
return false;
}

const secondDegreeTerms = node.args.filter(isPolynomialTermOfDegree('2'));
const firstDegreeTerms = node.args.filter(isPolynomialTermOfDegree('1'));
const constantTerms = node.args.filter(Node.Type.isConstant);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Check that there is one second degree term and at most one first degree
// term and at most one constant term
if (secondDegreeTerms.length !== 1 || firstDegreeTerms.length > 1 ||
constantTerms.length !== 1) {
constantTerms.length > 1) {
return false;
}

return true;
}

function isFirstDegree(node) {
if (Node.PolynomialTerm.isPolynomialTerm(node)) {
const polyTerm = new Node.PolynomialTerm(node);
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return false;
}
return false;

return true;
}

function isSecondDegree(node) {
if (Node.PolynomialTerm.isPolynomialTerm(node)) {
const polyTerm = new Node.PolynomialTerm(node);
const exponent = polyTerm.getExponentNode(true);
return exponent && exponent.value === '2';
}
return false;
function isPolynomialTermOfDegree(degree) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return function(node) {
if (Node.PolynomialTerm.isPolynomialTerm(node)) {
const polyTerm = new Node.PolynomialTerm(node);
const exponent = polyTerm.getExponentNode(true);
return exponent && exponent.value === degree;
}
return false;
};
}

module.exports = isQuadratic;
221 changes: 163 additions & 58 deletions lib/factor/factorQuadratic.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
'use strict';

const ChangeTypes = require('../ChangeTypes');
const checks = require('../checks');
const ConstantFactors = require('./ConstantFactors');
const ChangeTypes = require('../ChangeTypes');
const flatten = require('../../lib/util/flattenOperands');
const Negative = require('../Negative');
const Node = require('../node');

// Given a node, will check if it's in the form of a quadratic equation
// `ax^2 + bx + c`, and
// if it is, will factor it using on of the following rules:
// if it is, will factor it using one of the following rules:
// - Factor out the symbol e.g. x^2 + 2x -> x(x + 2)
// - Difference of squares e.g. x^2 - 4 -> (x+2)(x-2)
// - Perfect square e.g. x^2 + 2x + 1 -> (x+1)^2
// - Sum/product rule e.g. x^2 + 3x + 2 -> (x+1)(x+2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love these examples 😍

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

// get a, b and c
let firstTermNode, secondTermNode, constantTermValue;
let symbol, aValue, bValue, cValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (const term of node.args) {
if (Node.Type.isConstant(term)) {
constantTermValue = term.eval();
cValue = term.eval();
}
else if (Node.PolynomialTerm.isPolynomialTerm(term)) {
const polyTerm = new Node.PolynomialTerm(term);
const exponent = polyTerm.getExponentNode(true);
if (exponent.value === '2') {
firstTermNode = polyTerm;
symbol = polyTerm.getSymbolNode();
aValue = polyTerm.getCoeffValue();
}
else if (exponent.value === '1') {
secondTermNode = polyTerm;
bValue = polyTerm.getCoeffValue();
}
else {
return Node.Status.noChange(node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when would this catch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we take em out then?

Expand All @@ -44,84 +44,189 @@ function factorQuadratic(node) {
}
}

if (!firstTermNode || !constantTermValue) {
if (!symbol || !aValue) {
return Node.Status.noChange(node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when would this catch?

Copy link
Collaborator Author

@aelnaiem aelnaiem Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't. In all these cases, it's easy to account for it so I add these checks.

}

const symbol = firstTermNode.getSymbolNode();
const firstTermCoeffValue = firstTermNode.getCoeffValue();
const firstTermCoeffRootValue = Math.sqrt(Math.abs(firstTermCoeffValue));
const firstTermCoeffRootNode = Node.Creator.constant(firstTermCoeffRootValue);
let constantTermRootValue = Math.sqrt(Math.abs(constantTermValue));
let constantTermRootNode = Node.Creator.constant(constantTermRootValue);
let negate = false;
if (aValue < 0) {
negate = true;
aValue = -aValue;
bValue = -bValue;
cValue = -cValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

let status;

// factor just the symbol e.g. x^2 + 2x -> x(x + 2)
status = factorSymbol(node, symbol, aValue, bValue, cValue, negate);
if (status.hasChanged()) {
return status;
}

// factor difference of squares e.g. x^2 - 4
status = factorDifferenceOfSquares(node, symbol, aValue, bValue, cValue, negate);
if (status.hasChanged()) {
return status;
}

// factor perfect square e.g. x^2 + 2x + 1
status = factorPerfectSquare(node, symbol, aValue, bValue, cValue, negate);
if (status.hasChanged()) {
return status;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (status.hasChanged()) {
return status;
}

// TODO: quadratic formula
// a(x - (-b + sqrt(b^2 - 4ac)) / 2a)(x - (-b - sqrt(b^2 - 4ac)) / 2a)

return Node.Status.noChange(node);
}

// Will factor the node if it's in the form of ax^2 + bx
function factorSymbol(node, symbol, aValue, bValue, cValue, negate) {
if (bValue && !cValue) {
const aNode = Node.Creator.constant(aValue);
const bNode = Node.Creator.constant(bValue);

let polyTerm = Node.Creator.polynomialTerm(
symbol, null, aNode);
const paren = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, bNode]));

if (negate) {
polyTerm = Negative.negate(polyTerm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add tests for this case? I think there are some bugs and/or weird formatting that might happen

}

const newNode = Node.Creator.operator('*', [symbol, paren], true);
return Node.Status.nodeChanged(ChangeTypes.FACTOR_SYMBOL, node, newNode);
}

return Node.Status.noChange(node);
}

// Will factor the node if it's in the form of ax^2 - c, and the aValue
// and cValue are perfect squares
// e.g. 4x^2 - 4 -> (2x + 2)(2x - 2)
function factorDifferenceOfSquares(node, symbol, aValue, bValue, cValue, negate) {
// check if difference of squares: are a and c squares and there is no b and c
// is negative
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add commas between conditions as well or something - because you use "and" within conditions ("are a and c squares"), and you also use it to separate conditions "squares and there is"

  // check if difference of squares: (i) abs(a) and abs(c) are squares, (ii) b = 0,
  // (iii) c is negative

if (!bValue && cValue) {
const aRootValue = Math.sqrt(Math.abs(aValue));
const cRootValue = Math.sqrt(Math.abs(cValue));

// check if difference of squares: are a and c squares and there is no b
if (!secondTermNode) {
// must be a difference of squares
if (constantTermValue < 0 &&
firstTermCoeffRootValue % 1 === 0 &&
constantTermRootValue % 1 === 0) {
const polyTerm = Node.Creator.polynomialTerm(symbol, null, firstTermCoeffRootNode);
const firstParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, constantTermRootNode]));
if (Number.isInteger(aRootValue) &&
Number.isInteger(cRootValue) &&
cValue < 0) {

const aRootNode = Node.Creator.constant(aRootValue);
const cRootNode = Node.Creator.constant(cRootValue);

const polyTerm = Node.Creator.polynomialTerm(
symbol, null, aRootNode);
let firstParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, cRootNode]));
const secondParen = Node.Creator.parenthesis(
Node.Creator.operator('-', [polyTerm, constantTermRootNode]));
Node.Creator.operator('-', [polyTerm, cRootNode]));

if (negate) {
firstParen = Negative.negate(firstParen);
}

// create node in difference of squares form
const newNode = Node.Creator.operator('*', [firstParen, secondParen], true);
return Node.Status.nodeChanged(
ChangeTypes.FACTOR_DIFFERENCE_OF_SQUARES, node, newNode);
}
}
else {
// check if perfect square: are a and c squares and b = 2*sqrt(a)*sqrt(c)

// if the second term is negative, then the constant in the parens is subtracted
// i.e. x^2 -2x + 1 -> (x-1)^2
const secondTermCoeffValue = secondTermNode.getCoeffValue();
if (secondTermCoeffValue < 0) {
constantTermRootValue = constantTermRootValue * -1;
constantTermRootNode = Negative.negate(constantTermRootNode);

return Node.Status.noChange(node);
}

// Will factor the node if it's in the form of ax^2 + bx + c, where a and c
// are perfect squares and b = 2*sqrt(a)*sqrt(c)
// e.g. x^2 + 2x + 1 -> (x + 1)^2
function factorPerfectSquare(node, symbol, aValue, bValue, cValue, negate) {
// check if perfect square: are a and c squares and b = 2*sqrt(a)*sqrt(c)
if (bValue && cValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you should return early in these functions instead of nesting it all in an if

e.g.

if (!bValue || !cValue) {
  return Node.Status.noChange(node);
}
...

const aRootValue = Math.sqrt(Math.abs(aValue));
let cRootValue = Math.sqrt(Math.abs(cValue));

// if the second term is negative, then the constant in the parens is
// subtracted: e.g. x^2 - 2x + 1 -> (x - 1)^2
if (bValue < 0) {
cRootValue = cRootValue * -1;
}

const perfectProduct = 2 * firstTermCoeffRootValue * constantTermRootValue;
if (firstTermCoeffRootValue % 1 === 0 &&
constantTermRootValue % 1 === 0 &&
secondTermCoeffValue === perfectProduct) {
const polyTerm = Node.Creator.polynomialTerm(symbol, null, firstTermCoeffRootNode);
const paren = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, constantTermRootNode]));
// apply the perfect square test
const perfectProduct = 2 * aRootValue * cRootValue;
if (Number.isInteger(aRootValue) &&
Number.isInteger(cRootValue) &&
bValue === perfectProduct) {

const aRootNode = Node.Creator.constant(aRootValue);
const cRootNode = Node.Creator.constant(cRootValue);

const polyTerm = Node.Creator.polynomialTerm(
symbol, null, aRootNode);
let paren = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, cRootNode]));
const exponent = Node.Creator.constant(2);

if (negate) {
paren = Negative.negate(paren);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// create node in perfect square form
const newNode = Node.Creator.operator('^', [paren, exponent]);
return Node.Status.nodeChanged(
ChangeTypes.FACTOR_PERFECT_SQUARE, node, newNode);
}
else if (firstTermCoeffValue === 1) {
// try sum/product rule: find a factor pair of c that adds up to b
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);
const firstParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, Node.Creator.constant(pair[0])]));
const secondParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, Node.Creator.constant(pair[1])]));

// create a node in the general factored form for expression
const newNode = Node.Creator.operator('*', [firstParen, secondParen], true);
return Node.Status.nodeChanged(
ChangeTypes.FACTOR_SUM_PRODUCT_RULE, node, newNode);
}

return Node.Status.noChange(node);
}

// Will factor the node if it's in the form of x^2 + bx + c if a is 1, by
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit confusing that you say a is 1 when there's no a

maybe Will factor the node if it's in the form of x^2 + bx + c (i.e. a is 1), by....

// applying the sum product rule, that is finding factors of c that add up to
// b.
// e.g. x^2 + 3x + 2 -> (x + 1)(x + 2)
function factorSumProductRule(node, symbol, aValue, bValue, cValue, negate) {
if (aValue === 1 && bValue && cValue) {
// try sum/product rule: find a factor pair of c that adds up to b
const factorPairs = ConstantFactors.getFactorPairs(cValue, true);
for (const pair of factorPairs) {
if (pair[0] + pair[1] === bValue) {
const aRootValue = Math.sqrt(Math.abs(aValue));
const aRootNode = Node.Creator.constant(aRootValue);

const polyTerm = Node.Creator.polynomialTerm(
symbol, null, aRootNode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Node.Creator.polynomialTerm(symbol)

let firstParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, Node.Creator.constant(pair[0])]));
const secondParen = Node.Creator.parenthesis(
Node.Creator.operator('+', [polyTerm, Node.Creator.constant(pair[1])]));

if (negate) {
firstParen = Negative.negate(firstParen);
}

// create a node in the general factored form for expression
const newNode = Node.Creator.operator('*', [firstParen, secondParen], true);
return Node.Status.nodeChanged(
ChangeTypes.FACTOR_SUM_PRODUCT_RULE, node, newNode);
}
}
}

// TODO: quadratic formula
// a(x - (-b + sqrt(b^2 - 4ac)) / 2a)(x - (-b - sqrt(b^2 - 4ac)) / 2a)

return Node.Status.noChange(node);
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

module.exports = factorQuadratic;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this functionality won't exposed in mathsteps yet, right?

should we add it to the top level index.js, or wait until we figure out what a more general factor interface will look like?

4 changes: 2 additions & 2 deletions lib/node/Creator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

or is that more confusing?

let polyTerm = symbol;
if (exponent) {
polyTerm = this.operator('^', [polyTerm, exponent]);
}
if (coeff && coeff.value !== '1') {
if (coeff && (coeffIsOne || parseFloat(coeff.value) !== 1)) {
if (NodeType.isConstant(coeff) &&
parseFloat(coeff.value) === -1 &&
!coeffIsNegOne) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add comment for what true means


newNode.args[i].changeGroup = changeGroup;
node.args[i].changeGroup = changeGroup; // note that this is the "oldNode"
Expand Down Expand Up @@ -120,6 +121,7 @@ function addNegativeOneCoefficient(node) {
polyTerm.getSymbolNode(),
polyTerm.getExponentNode(),
polyTerm.getCoeffNode(),
false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment for what this means maybe

true /* explicit -1 coefficient */);

node.args[i].changeGroup = changeGroup; // note that this is the "oldNode"
Expand Down
Loading