-
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 |
---|---|---|
|
@@ -12,13 +12,14 @@ function isQuadratic(node) { | |
return false; | ||
} | ||
|
||
// get the number of symbols in the expression and ensure there's only one | ||
const symbolSet = Symbols.getSymbolsInExpression(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. 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 secondDegreeTerms = node.args.filter(isPolynomialTermOfDegree(2)); | ||
const firstDegreeTerms = node.args.filter(isPolynomialTermOfDegree(1)); | ||
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 |
||
|
||
// Check that there is one second degree term and at most one first degree | ||
|
@@ -29,19 +30,22 @@ function isQuadratic(node) { | |
} | ||
|
||
// check that there are no terms that don't fall into these groups | ||
if (secondDegreeTerms.length + firstDegreeTerms.length + constantTerms.length !== node.args.length) { | ||
if ((secondDegreeTerms.length + firstDegreeTerms.length + | ||
constantTerms.length) !== node.args.length) { | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
// Given a degree, returns a function that checks if a node | ||
// is a polynomial term of the given degree. | ||
function isPolynomialTermOfDegree(degree) { | ||
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 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 exponent && parseFloat(exponent.value) === degree; | ||
} | ||
return false; | ||
}; | ||
|
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.
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