-
Notifications
You must be signed in to change notification settings - Fork 276
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
Changes from 2 commits
d9633f4
988b98e
a527552
93adba6
d71c753
f7ee0de
ce97fc3
915f539
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
'use strict'; | ||
|
||
const Node = require('../node'); | ||
|
||
// TODO(ael) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docstring |
||
function isQuadratic(node) { | ||
if (!Node.Type.isOperator(node) || node.op !== '+') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can now do |
||
return false; | ||
} | ||
|
||
if (node.args.length > 3) { | ||
return false; | ||
} | ||
|
||
const secondDegreeTerms = node.args.filter(isSecondDegree); | ||
const firstDegreeTerms = node.args.filter(isFirstDegree); | ||
const constantTerms = node.args.filter(Node.Type.isConstant); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
if (secondDegreeTerms.length !== 1 || firstDegreeTerms.length > 1 || | ||
constantTerms.length !== 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm why'd you choose this combination? I agree that there should be a secondDegreeTerm for sure but what about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmmmmm. Interesting. You're right :) |
||
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'; | ||
} | ||
return false; | ||
} | ||
|
||
function isSecondDegree(node) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you just have a general function |
||
if (Node.PolynomialTerm.isPolynomialTerm(node)) { | ||
const polyTerm = new Node.PolynomialTerm(node); | ||
const exponent = polyTerm.getExponentNode(true); | ||
return exponent && exponent.value === '2'; | ||
} | ||
return false; | ||
} | ||
|
||
module.exports = isQuadratic; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
'use strict'; | ||
|
||
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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one of |
||
// - 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. love these examples 😍 |
||
// - TODO: quadratic formula | ||
function factorQuadratic(node) { | ||
node = flatten(node); | ||
if (!checks.isQuadratic(node)) { | ||
return Node.Status.noChange(node); | ||
} | ||
|
||
// get a, b and c | ||
let firstTermNode, secondTermNode, constantTermValue; | ||
for (const term of node.args) { | ||
if (Node.Type.isConstant(term)) { | ||
constantTermValue = 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; | ||
} | ||
else if (exponent.value === '1') { | ||
secondTermNode = polyTerm; | ||
} | ||
else { | ||
return Node.Status.noChange(node); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when would this catch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we take em out then? |
||
} | ||
} | ||
else { | ||
return Node.Status.noChange(node); | ||
} | ||
} | ||
|
||
if (!firstTermNode || !constantTermValue) { | ||
return Node.Status.noChange(node); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when would this catch? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooohhh in the quadratic check can you make sure they have the same symbol? and add a test for that too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
const firstTermCoeffValue = firstTermNode.getCoeffValue(); | ||
const firstTermCoeffRootValue = Math.sqrt(Math.abs(firstTermCoeffValue)); | ||
const firstTermCoeffRootNode = Node.Creator.constant(firstTermCoeffRootValue); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this name is probably ok? but might be confusing at first I thought it was an nthroot fucntion node There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a better name in mind? I agree that the naming is annoying. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if you wait until you need them to create/name them, it'll be more obvious what they do because the context |
||
let constantTermRootValue = Math.sqrt(Math.abs(constantTermValue)); | ||
let constantTermRootNode = Node.Creator.constant(constantTermRootValue); | ||
|
||
// check if difference of squares: are a and c squares and there is no b | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... and c is negative |
||
if (!secondTermNode) { | ||
// must be a difference of squares | ||
if (constantTermValue < 0 && | ||
firstTermCoeffRootValue % 1 === 0 && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make a helper function to make it clear that you're checking to see if it's an integer? or you could use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger |
||
constantTermRootValue % 1 === 0) { | ||
const polyTerm = Node.Creator.polynomialTerm(symbol, null, firstTermCoeffRootNode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you split this line into 2 lines? (and everywhere else you have a line like this) |
||
const firstParen = Node.Creator.parenthesis( | ||
Node.Creator.operator('+', [polyTerm, constantTermRootNode])); | ||
const secondParen = Node.Creator.parenthesis( | ||
Node.Creator.operator('-', [polyTerm, constantTermRootNode])); | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternatively, you can just make not sure if that's better or not I have a slight preference for this because it becomes more obvious why you need it |
||
} | ||
|
||
const perfectProduct = 2 * firstTermCoeffRootValue * constantTermRootValue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
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])); | ||
const exponent = Node.Creator.constant(2); | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we know that the coeff is 1, so the polyTerm is just the symbol right? |
||
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); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// TODO: quadratic formula | ||
// a(x - (-b + sqrt(b^2 - 4ac)) / 2a)(x - (-b - sqrt(b^2 - 4ac)) / 2a) | ||
|
||
return Node.Status.noChange(node); | ||
} | ||
|
||
module.exports = factorQuadratic; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
'use strict'; | ||
|
||
const assert = require('assert'); | ||
const math = require('mathjs'); | ||
|
||
const flatten = require('../../lib/util/flattenOperands'); | ||
const checks = require('../../lib/checks'); | ||
|
||
function testIsQuadratic(exprString, result) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just merged an update to add |
||
it(exprString + ' ' + result, function () { | ||
assert.deepEqual( | ||
checks.isQuadratic(flatten(math.parse(exprString))), | ||
result); | ||
}); | ||
} | ||
|
||
describe('isQuadratic', function () { | ||
const tests = [ | ||
['2 + 2', false], | ||
['x', false], | ||
['x^2 - 4', true], | ||
['x^2 + 2x + 1', true], | ||
['x^2 - 2x + 1', true], | ||
['x^2 + 3x + 2', true], | ||
['x^2 - 3x + 2', true], | ||
['x^2 + x - 2', true], | ||
['x^2 + 4', true], | ||
['x^2 + 4x + 1', true], | ||
['x^2', false], | ||
['x^3 + x^2 + x + 1', false], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add tests for |
||
]; | ||
tests.forEach(t => testIsQuadratic(t[0], t[1])); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
'use strict'; | ||
|
||
const assert = require('assert'); | ||
const math = require('mathjs'); | ||
|
||
const flatten = require('../../lib/util/flattenOperands'); | ||
const print = require('../../lib/util/print'); | ||
|
||
const factorQuadratic = require('../../lib/factor/factorQuadratic'); | ||
|
||
function testFactorQuadratic(exprStr, outputStr) { | ||
it(exprStr + ' -> ' + outputStr, function () { | ||
assert.deepEqual( | ||
print(factorQuadratic(flatten(math.parse(exprStr))).newNode), | ||
outputStr); | ||
}); | ||
} | ||
describe('factorQuadratic', function () { | ||
const tests = [ | ||
['x^2 - 4', '(x + 2)(x - 2)'], | ||
['x^2 + 2x + 1', '(x + 1)^2'], | ||
['x^2 - 2x + 1', '(x - 1)^2'], | ||
['x^2 + 3x + 2', '(x + 1)(x + 2)'], | ||
['x^2 - 3x + 2', '(x - 1)(x - 2)'], | ||
['x^2 + x - 2', '(x - 1)(x + 2)'], | ||
['x^2 + 4', 'x^2 + 4'], | ||
['x^2 + 4x + 1', 'x^2 + 4x + 1'], | ||
]; | ||
tests.forEach(t => testFactorQuadratic(t[0], t[1])); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄 )