Factoring Trinomials and Difference of Squares by aliang8 · Pull Request #41 · semantic-math/math-rules

aliang8 · 2017-06-02T02:09:23Z

PERFECT_SQUARE_TRINOMIAL and FACTOR_TRINOMIAL logic is somewhat redundant, not sure if we should remove it and just have one rule. Code should be refactored if possible.

k4b7 · 2017-06-13T02:00:52Z

lib/rules.js

+// assume monic polynomial
+// TODO: add min and max to evaluate
+
+export const FACTOR_SYMBOL = defineRuleString('#a^#b_0 + ...', '#a^#eval(min(#b_0, ...)) (#a^#eval(#b_0 - min(#b_0, ...)) + ...)', {b: query.isNumber})


Neato! Could you provide an example what this does?

Ahhh, this doesn't exactly work yet. This rule only applies to addition (x^2 + x^3 + x^4 -> x^2(x^0 + x^1 + x^2). Not exactly sure if we want two separate rules for addition and subtraction or if there is a way to do both in a rule string. I also need the min and max functions to be exposed in math-evaluator.

I think this should work with subtraction as is. Have you tried writing a test case with subtraction?

Yeah, I need min and max to support nary parameters.

k4b7 · 2017-06-13T02:05:11Z

lib/rules.js

+    (node) => {
+        const isFactorable = canApplyRule(FACTOR_SUM_PRODUCT_RULE, node)
+        if (isFactorable) {
+            const result = applyRule(FACTOR_SUM_PRODUCT_RULE, node)


k4b7 · 2017-06-13T02:06:40Z

lib/rules.js

 // e.g nthRoot(9, 2) -> 3
 export const NTH_ROOT_VALUE = defineRuleString('nthRoot(#a, #b)', '#eval(nthRoot(#a, #b))', {a: query.isNumber, b: query.isNumber})

+// FACTOR


Can you put these in their own file? I'd like eventually rename this file to be simple-rules.js and have it contain only rules which can be full expressed with patterns.

k4b7 · 2017-06-13T02:09:47Z

lib/rules.js

+                let isFactorable = false
+                const {constants, coefficientMap} = getCoefficientsAndConstants(node)
+                const variables = Object.keys(coefficientMap).map(key => parse(key))
+                const coefficients = Object.keys(coefficientMap).map(key => {return coefficientMap[key]})


const coeffs = Object.keys(coeffMap).map(key => coeffMap[key][0])

This keeps it to a single line and you don't have to do coeffs[1][0] later you can just do coeffs[1].

k4b7 · 2017-06-13T02:14:52Z

lib/rules.js

+            const isTrinomial =
+                  isPolynomialTerm(firstTerm)
+                  && isPolynomialTerm(secondTerm)
+                  && (query.isNumber(thirdTerm) || isPolynomialTerm(thirdTerm))


We should probably handle things like 1 + 2x + x^2 as well... maybe we should have a transform to order terms in a polynomial in descending order. This can be a TODO.

Yup I was thinking to go with the second approach.

k4b7 · 2017-06-13T02:22:27Z

lib/rules.js

+    (node) => {
+        const {constants, coefficientMap} = getCoefficientsAndConstants(node)
+        const variables = Object.keys(coefficientMap).map(key => parse(key))
+        const coefficients = Object.keys(coefficientMap).map(key => {return coefficientMap[key]})


return isn't necessary.

k4b7 · 2017-06-13T02:26:44Z

lib/rules.js

+        for (var i in firstFactors){
+            for (var j in thirdFactors) {
+                const l1 = firstFactors[i]
+                const l2 = thirdFactors[j]


I have no idea what's going on here. Can you add a comment explaining this algorithm?

k4b7 · 2017-06-13T02:27:56Z

lib/rules.js

+                   build.apply(
+                       'mul',
+                       [build.number(factor[0]), ...firstPoly.map(
+                           term =>


term -> factor

k4b7 · 2017-06-13T02:29:00Z

lib/rules.js

+        }
+
+        const result = build.applyNode
+        ('mul',


All of the these implicit muls can be build using build.implicitMul.

k4b7 · 2017-06-13T02:29:39Z

lib/rules.js

+        }
+        if (combo == []) {
+            throw new Error('cannot factor polynomial')
+        }


If feels weird that canApplyRule might be successful, but applyRule might still fail.

Good point, I have to fix this.

Not entirely sure how to without replicating the code for rewriteFn.

I would extract a common function. Obviously it's not ideal to run the same code twice, but I'd rather maintain the invariant that if canApplyRule returns true that applyRule will succeed.

k4b7 · 2017-06-18T21:54:18Z

lib/factor-rules.js

+
+const getExponent = (node) => {
+    if (query.isNumber(node)) {
+        return 0


This seems like a weird value to return for a number. I would expect the exponent to be 1. Maybe there should be an if statement before calling getExponent to check if the node is a number.

Oh, the getExponent function is to get the degree of x for a given polynomial term. So for instance, 6x^2 + 3x + 1, getExponent on 1 would return 0 because it is 1x^0. Maybe the function name itself is a bit misleading?

k4b7 · 2017-06-18T21:55:38Z

lib/factor-rules.js

+    } else if (query.isPow(node)) {
+        return query.getValue(node.args[1])
+    } else if (query.isMul(node)){
+        return getExponent(node.args[1])


Shouldn't this be return Math.max(...node.args.map(getExponent))?

It's okay to limit ourselves to single variables for now, but we should have a TODO somewhere.

k4b7 · 2017-06-18T21:56:38Z

lib/factor-rules.js

+}
+
+// e.g. 2 + 3x^2 + 3x - 4x^3 -> -4x^3 + 3x^2 + 3x + 2
+export const REARRANGE_TERMS = defineRule(


This doesn't take into account polynomials with multiple variables. Maybe add a TODO.

k4b7 · 2017-06-18T22:00:43Z

lib/factor-rules.js

+
+  3. Cross multiply the factors and check if
+  the result matches one of the eight conditions
+  (the second factor shld also multiply to equal the thirdCoef)


shld --> should

k4b7 · 2017-06-18T22:04:13Z

lib/factor-rules.js

+
+  Check: 1 * 2 + 6 * 1 ?= 7 so on and so forth
+  Once we reach 2 * 2 + 3 * 1 ?= 7, we have found a match.
+  (Note: 3 * 1 = 3 satisfying the second condition)


I'm confused about this note.... don't we want to check if the multiple the first candidate coefficients results in the first coefficient of the trinomial and multiplying the both of the second candidate numbers results in the constant of the trinomial. In this case 2 * 3 = 6 and 2 * 1 = 2 so it works out.

You already know that 2 * 3 = 6 and 2 * 1 = 2. It comes from the fact that those are the factors of 6 and 2 respectively (pairs that we got from firstFactors and thirdFactors). The note here is just to make sure we don't have an erroneous combo / solution (this happened a few times in my test cases).

I need to spend some more time with this to really understand what's going on. I have some of pieces but not everything. This is eventually going to get re-reviewed when it gets moved over to mathsteps so I'm okay with merging this.

Awesome, let me know when you merge this, so I can update my other PRs.

k4b7 · 2017-06-18T22:06:23Z

lib/factor-rules.js

+    }
+)
+
+const getFactors = (num) => {


This should maybe be called getFactorPairs or add a comment describing what it does. I was confused b/c I thought calling getFactors(12) would return [1, 2, 3, 4, 6, 12] but in fact it returns [[1, 12], [2, 6], [3, 4]].

k4b7 · 2017-06-18T22:10:15Z

lib/factor-rules.js

+        ? query.isMul(variables[2])
+        ? variables[2].args
+        : [variables[2]]
+        : null


Can you rewrite this ternary as an if-else?

k4b7 · 2017-06-18T22:10:20Z

lib/factor-rules.js

+        query.getValue(coeffs[2])
+    : constants[0]
+    ? query.getValue(constants[0])
+    : null


k4b7 · 2017-06-18T22:12:09Z

lib/factor-rules.js

+  At this point, we find that the correct combination is
+  : [2,3] and [1,2] -> (2x + 3)(1x + 2)
+*/
+const factor_trinomial_helper = (node) => {


Does this handle cases where the lead coefficient is negative?

We assume that lead coefficient is positive because it should have been factored out previously by another rule.

k4b7 · 2017-06-18T22:20:12Z

lib/factor-rules.js

+              ? query.isMul(variables[2])
+              ? variables[2].args
+          : [variables[2]]
+        : null


The indentation is a little off here. It should probably be:

const thirdPoly = variables[2] ? query.isMul(variables[2]) ? variables[2].args : [variables[2]] : null

even that is hard to parse... since the default is null maybe do:

let thirdPoly = null; if (variables[2]) { if (query.isMul(variables[2]) { thirdPoly = variables[2].args } else { thirdPoly = [variables[2]] } }

k4b7 · 2017-06-18T22:20:47Z

lib/factor-rules.js

+      ? query.isMul(variables[2])
+          ? variables[2].args
+      : [variables[2]]
+          : null


Please rewrite as if-else statements.

aliang8 added 30 commits April 30, 2017 13:48

added rules for patterns w/o constraints

04bb0ee

added rules test, some errors

f37f48c

rm file

5745443

rm file

b462d8a

format

3ca4c7f

fixed tabing, remove comments, added rules, need to check variables

adcbc10

added extra test cases, not done

3cb89d4

not sure what's up with these tests

75ab164

finished adding cases, still some errors

3beef3e

fixed merge errors

c35f087

one last test

20bc705

fixed tests and yarn

336c5c3

done

3dbb8cd

removed lint, added TODO

2335275

merging

d91eb26

rebasing

7fdbd7e

merge

e8e86a1

merged

7a3e271

merge

fbd4938

merge

3ea7468

merged

96d2bb9

addingPolynomials

caa0ae0

fixed comments

14a091f

added more tests, one of them fails

aa21a98

remove . file

08b3ca1

merge

03ffdf1

remove alphabetize

648f0f7

fixed comments

632b525

removed comment

7403644

starting to work on LCM

2cb2f76

aliang8 added 2 commits June 11, 2017 18:02

change yarn.lock

80bf390

remove dot file

6a29516

k4b7 requested changes Jun 13, 2017

View reviewed changes

aliang8 added 12 commits June 13, 2017 12:06

fixing changes

a9f5f86

separate files

e81b234

rm package-lock

ab710f8

added explaination of factoring rule

bc60b87

starting work on rearrange_terms

ddd2192

refactored code and test

06ab528

added rerranging term rule

4fb7bd4

updated yarn

0dd219b

revert yarn.lock

2558b48

change index.js

1012628

merge with master

d0b5364

revert yarn.lock to masters

984557f

k4b7 reviewed Jun 18, 2017

View reviewed changes

lib/factor-rules.js Outdated

query.getValue(coeffs[2])

: constants[0]

? query.getValue(constants[0])

: null

Copy link

Member

k4b7 Jun 18, 2017

Choose a reason for hiding this comment

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

same

k4b7 reviewed Jun 18, 2017

View reviewed changes

aliang8 added 3 commits June 18, 2017 20:48

fixed ternaries, added TODO

4b45333

rules-list

b75e4b4

merge

8971f88

k4b7 approved these changes Jun 20, 2017

View reviewed changes

Conversation

aliang8 commented Jun 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliang8 Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliang8 Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

aliang8 Jun 19, 2017 •

edited

Loading

aliang8 Jun 19, 2017 •

edited

Loading