Multiplying polynomials by aliang8 · Pull Request #34 · semantic-math/math-rules

aliang8 · 2017-05-21T18:13:52Z

Added rule for multiplying polynomials.

x^2 * x^1', '1 x^3

k4b7

Please remove all changes that are already part #32. Also, MULTIPLY_POLYNOMIALS should be split up.

k4b7 · 2017-05-21T19:12:39Z

lib/rules.js

 export const NEGATION = defineRuleString('--#a', '#a')

+export const REARRANGE_COEFF = defineRuleString('#b * #a', '#a #b', {a: query.isNumber, b: isPolynomialTerm})
+


This shouldn't be included b/c it's part of the other diff.

k4b7 · 2017-05-21T19:13:03Z

lib/rules.js

+                }
+            }
+        }
+                              )


indentation

k4b7 · 2017-05-21T19:16:47Z

lib/rules.js

 export const QUOTIENT_RULE = defineRuleString('#a^#p / #a^#q', '#a^(#p - #q)')

+// e.g. x^2 * x^2 -> x^4
+export const MULTIPLY_POLYNOMIALS = defineRule(


mathsteps has no MULTIPLY_POLYNOMIALS... it has a set of steps. We should implement the steps in https://github.com/socraticorg/mathsteps/blob/master/lib/ChangeTypes.js#L60-L69 unless there's a strong reason why we shouldn't.

k4b7 · 2017-05-21T19:17:35Z

lib/rules.js

 // COLLECT AND COMBINE
 export {default as COLLECT_LIKE_TERMS} from './rules/collect-like-terms'

+export const FRACTIONAL_POLYNOMIALS = defineRule(


This is from the other PR as well. It shouldn't be part of this PR.

k4b7 · 2017-05-21T19:19:51Z

test/rules_test.js

+        //['x^2 * x', 'x^2 * x^1'],
+        //['x^2 * 2 * x * x', ''],
+        //['x + 2 * x', '']
+    ])


Fix test cases and uncomment.

k4b7

Do we need both ADD_EXPONENT_OF_ONE_HELPER and ADD_EXPONENT_OF_ONE. I feel like ADD_EXPONENT_OF_ONE_HELPER is simpler and should do everything that we need for this rule.

k4b7 · 2017-05-23T04:22:59Z

test/rules_test.js

-        ['x^2 * 2 * x * x', ''],
-        ['x + 2 * x', '']
+        ['x^2 * 2 * x * x', 'x^2 * 2 * x^1 * x^1'],
+        ['2x + 3x^2 * x y z', '2 x + 3 x^2 * (x^1 * y^1 * z^1)'],


We should maintain the implicit flag on x y z. I would expect the output of this test to be:

2 x^1 + 3 x^2 * x^1 y^1 z^1

The first term should have a ^1 added as well, shouldn't it?

k4b7 · 2017-05-23T04:23:45Z

lib/rules.js

+            'mul',
+            node.args.map(arg => {
+                if (canApplyRule(ADD_EXPONENT_OF_ONE_HELPER, arg)) {
+                    console.log("1st")


k4b7 · 2017-05-23T04:24:07Z

lib/rules.js

                }
-            }
-        })
+                return arg


Can you put this in an else block?

k4b7 · 2017-05-23T04:25:57Z

lib/rules.js

+        if (query.isMul(node)) {
+            node.args.some(arg => {
+                hasConstantVariable = query.isIdentifier(arg)
+            })


This is a confusing construct... the following would be clearer:

const hasConstantVar = query.isMul(node) && node.args.some(query.isIdentifier)

k4b7 · 2017-05-23T04:26:51Z

lib/rules.js

+        } 
+
+        return (hasConstantVariable) ?
+            {node} : null


For ternaries please put them on a single line or the following:

return hasConstantVariable ? {node} : null

k4b7 · 2017-05-23T04:30:21Z

lib/rules.js

+                    return build.applyNode('pow', [arg, build.numberNode(1)])
+                }
+                return arg
+            }), {implicit: true}


{implicit: node.implicit}

aliang8 · 2017-05-23T19:02:01Z

The helper rule does it for a single polynomial, specially when they're an implicit multiplication of terms (e.g 6xyz), while the rule itself applies the helper to all the terms and handles x. I think it would be easier with the helper, but if you feel like it is unnecessary , I could try to find another way to combine them.

k4b7 · 2017-05-25T02:06:01Z

test/rules_test.js

+        ['x^2 * x^1', '(1 * 1) (x^2 * x^1)'],
+        ['3x^2 * x^2', '(3 * 1) (x^2 * x^2)'],
+        ['x^3 * 2y^2', '(1 * 2) (x^3 * y^2)'],
+        ['x^3 + 2x + 3x^1 * 5x^1', 'x^3 + 2 x + (3 * 5) (x^1 * x^1)'],


Nice test cases. I was thinking it'd be nice to be able to check similar inputs which fail to meet the rule's criteria. I've added a task for it #36.

Can you add a test case involving the product of three polynomials that are the same?

k4b7 · 2017-05-25T02:11:05Z

test/rules_test.js

+        ['3x^2 * x^2', '3 x^4'],
+        ['x^3 * 2y^2', '2 (x^3 y^2)'],
+        ['x^3 + 2x + 3x^1 * 5x^1', 'x^3 + 2 x + 15 x^2'],
+    ])


This does a couple of steps:

multiplying the coefficients

actually doing evaluating the product

Mathsteps still wants all of the steps. I feel like we need to take a step back and think about we can combine multiple steps. We could use MULTIPLY_COEFFICIENTS + SIMPLIFY_ARITHMETIC, but we currently don't have a way to tell SIMPLIFY_ARITHMETIC that we want it to simplify only the nodes that MULTIPLY_COEFFICIENTS manipulated.

k4b7 · 2017-05-25T02:13:54Z

lib/rules.js

+
 // e.g. x^2 * x -> x^2 * x^1
-// export const ADD_EXPONENT_OF_ONE = ...
+export const ADD_EXPONENT_OF_ONE = defineRule(


This rule could be implemented without the helper once we address #23. Can you add a TODO here?

k4b7 · 2017-05-25T02:16:20Z

lib/rules.js

+                if (query.isIdentifier(arg)){
+                    return build.applyNode('pow', [arg, build.numberNode(1)])
+                }
+                return arg


Can you use an else block here or change this to ternary statement?

k4b7 · 2017-05-25T02:21:27Z

lib/rules.js

+            const {constants, coefficientMap} = getCoefficientsAndConstants(node)
+            isMulOfPolynomials = Object.keys(coefficientMap).length > 1
+                || Object.keys(coefficientMap)
+                .some(key => coefficientMap[key].length > 1)


This is kind of confusing. The coefficientMap is designed for collecting like terms, but makes this task more complicated. Instead try using getVariableFactors.

getVariableFactors doesn't really help in the case of :
2x + 3x^2 * x y z => 2 x + 3 x^2 * x^1 y^1 z^1

It returns []

k4b7 · 2017-05-25T03:21:51Z

lib/rules.js

+// e.g. 3x^2 * 2x^2 -> (3 * 2)(x^2 * x^2)
+export const MULTIPLY_COEFFICIENTS = defineRule(
+    (node) => {
+        return canApplyRule(ADD_EXPONENT_OF_ONE, node) ? {node} : null


Shouldn't ADD_EXPONENT_OF_ONE only be matching things like x or x * x? This matching function though seems to be accepting x^2 which I would expect ADD_EXPONENT_OF_ONE to ignore.

I guess ADD_EXPONENT_OF_ONE_HELPER does what you want. ADD_EXPONENT_OF_ONE looks through the given expression and applies the helper where necessary.

k4b7 · 2017-06-18T21:43:32Z

lib/__test__/rule-list.test.js

    ])

+    suite('multiplying coefficients', rules.MULTIPLY_COEFFICIENTS, [
+        ['x^2 * y^2', '(1 * 1) (x^2 * y^2)'],


This feels like a really result. We end up with a more complicated expression that actually contains the original expression. Maybe check with Gen to see if we actually want this.

Looking at the tests in collectAndCombineSearch.test.js mathsteps doesn't bother showing the 1 coefficients.

['x^2 * x * x', ['x^2 * x^1 * x^1', 'x^(2 + 1 + 1)', 'x^4'], ], ['y * y^3', ['y^1 * y^3', 'y^(1 + 3)', 'y^4'], ], ['2x * x^2 * 5x', ['(2 * 5) * (x * x^2 * x)', '10 * (x * x^2 * x)', '10x^4'], '10x^4' ],

I'm okay with deferring the work to get this behavior until we move this code over to mathsteps. Can you add a TODO so that we can remind ourselves of this difference in behavior?

Fixed weird behavior in new commit.

k4b7 · 2017-06-20T01:30:15Z

lib/__test__/rule-list.test.js

-        ['x^2y^2z^2 * 2x^2', '(1 * 2) (x^2 * y^2 * z^2 * x^2)'],
-        ['3x^2 * x^2', '(3 * 1) (x^2 * x^2)'],
-        ['x^3 * y^2', '(1 * 1) (x^3 * y^2)'],
+        ['x^2 * y^2', 'x^2 * y^2'],


This might need tweak when it gets moved over to mathsteps b/c this should not be a change step b/c nothing changed but right now as long as rewriteNode returns a non-null value we assume that a change has occurred. We can take care of tweaking like that in mathsteps though.

k4b7 · 2017-06-20T01:32:48Z

lib/rule-list.js

+              ? build.implicitMul(build.mul(...coeffs), poly)
+              : coeffs.length == 1
+              ? build.implicitMul(coeffs[0], poly)
+              : poly


Please no nested ternaries. They're hard to understand and even harder understand with this formatting.

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 3 commits May 21, 2017 10:22

rules.js

56d4872

multiplyingPolynomials

e02838b

merge

06914d0

k4b7 requested changes May 21, 2017

View reviewed changes

aliang8 added 5 commits May 21, 2017 16:12

fixed comments

510afb7

working on adding one exponent

eb95348

exponent of one

2e67877

fixed adding exponent of one

b485d9e

remove comment

7784dd7

k4b7 requested changes May 23, 2017

View reviewed changes

aliang8 and others added 3 commits May 23, 2017 15:02

fixing comments

ef26741

fixing last comment

894bbbc

Merge branch 'master' into multiplyingPolynomials

d73a190

k4b7 requested changes May 27, 2017

View reviewed changes

aliang8 added 2 commits May 28, 2017 14:06

minor fixes

9741fbf

minor fixes

ba2d8f6

k4b7 mentioned this pull request May 30, 2017

Add option to apply a rule multiple times if the input pattern is matched multiple times #23

Open

aliang8 added 8 commits June 8, 2017 13:26

merge

da48fc1

merge

d5d9576

change rules

8bf1170

fixed test cases

9d07321

merge

5b4a5f2

merge with master

bb1dbd4

update multiplying polynomial functionality

0e1724a

remove random

2599741

k4b7 reviewed Jun 18, 2017

View reviewed changes

fixed weird behavior

26b3697

k4b7 reviewed Jun 20, 2017

View reviewed changes

		export const NEGATION = defineRuleString('--#a', '#a')

		export const REARRANGE_COEFF = defineRuleString('#b * #a', '#a #b', {a: query.isNumber, b: isPolynomialTerm})

Conversation

aliang8 commented May 21, 2017

Uh oh!

k4b7 left a comment

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!

k4b7 left a comment

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 commented May 23, 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!

aliang8 May 28, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aliang8 May 28, 2017 •

edited

Loading