Fractions #50

aliang8 · 2017-06-15T03:11:39Z

Fixed COMMON_DENOMINATORS to work with non integer denominators.

kevinbarabash · 2017-06-16T03:20:18Z

lib/rule-list.js

+            let sameDenom
+            // rule cannot apply if all of the terms are fractions
+            // and they all have the same denominator
+            if (terms.every(term => query.isDiv(term))) {


We should avoid requiring every single term b/c it prevents handling of common denominators of fractions surrounded by non-fraction terms, e.g.

1 + 2/x + 3/y + 4

I think you should be able to run matchNode on the current node with the #a_0 / #b_0 + ... as the pattern to get the start/end indexes of the first group of 2 or more consecutive fractions. You'll have the pattern to a matching function before you can use it with matchNode.

I see, I didn't consider using matchNode inside of a defineRule function. Wouldn't that pattern not take into consideration the case of non-fraction terms (1 + 2/x + 3/y + 4)?

kevinbarabash · 2017-06-16T03:24:49Z

lib/rule-list.js

+                (num, i) => build.mul(num,
+                                      build.number(LCM / denoms[i])))
+        } else {
+            newDenoms = build.mul(...denoms)


This isn't ideal, if all of the denominators are x then we'll make this overly complicated.

When will all the denominators be x? I don't think COMMON_DENOMINATORS will match that.

kevinbarabash · 2017-06-16T03:25:42Z

lib/rule-list.js

+    (node) => {
+        const terms = node.args
+        // an array storing the index where fraction is negative
+        const negatives = terms.map(term => query.isNeg(term))


It would be could if we could extract this logic for reuse. Don't worry about it for this review, but something to think about.

What logic are you referring to? The negatives array logic or the rule itself?

Computing the negatives array from the terms.

kevinbarabash · 2017-06-16T03:27:10Z

lib/__test__/rule-list.test.js

@@ -181,12 +188,14 @@ describe('rules', () => {

    suite('common denominators', rules.COMMON_DENOMINATOR, [
        ['2/6 + 1/4', '(2 * 2) / (6 * 2) + (1 * 3) / (4 * 3)'],
+        ['2/(3 + x) + 3/(2 + x)', '(2 * (2 + x)) / ((3 + x) * (2 + x)) + (3 * (3 + x)) / ((3 + x) * (2 + x))'],


There should be some tests with:

a mix of numeric denominators and variable denominators

repeated variable denominators

repeated numeric denominators

non-fractional terms in the sum

COMMON_DENOMINATORS won't match when the denominator is the same throughout regardless of whether they are variable or numeric. I'm not entirely sure how we want to handle non-fractional terms in the sum yet.

There are a couple of options:

don't include non-fractions when matching

include non-fractions, but convert them to fractions before computing the common denominator.

You should ask Gen for their input on this.

What happens if some of the fractions have the same denominator while others don't?

Yup good points. I had a test case that was commented out addressing this issue. I will ask for Gen's input.

What happens if some of the fractions have the same denominator while others don't?

It works fine in this scenario. I've added a test case for this. The only time when this rule doesn't match is when all the terms are fractions and they all have the same denominator.

I added a fix for it.

kevinbarabash · 2017-06-18T21:25:36Z

lib/rule-list.js

+                          ? build.div(term, build.number(1))
+                          : query.isNeg(term)
+                          ? term.args[0]
+                          : term)


This is super confusing. Can you rewrite this use if-else statements?

Yeah xD I realized. Sure.

kevinbarabash · 2017-06-18T21:29:52Z

lib/rule-list.js

+
+            let sameDenom
+            // rule cannot apply if all of the terms are fractions
+            // and they all have the same denominator


It might be worth moving this comment up to the top of the function so that people don't have to read the whole function to understand the restrictions.

kevinbarabash · 2017-06-18T21:30:49Z

lib/rule-list.js

+                const denom = getDenominator(terms[0])
+
+                sameDenom = terms.every(term =>
+                                        print(getDenominator(term)) == print(denom))


We might want to encapsulate this into an equals function in utils.js.

kevinbarabash · 2017-06-18T21:33:17Z

lib/rule-list.js

+    (node) => {
+        node = canApplyRule(CONVERT_TO_FRACTION, node)
+            ? applyRule(CONVERT_TO_FRACTION, node)
+            : node


We convert non-fraction terms to fractions here, but in the match function part of this rule we return false if any of the terms are non-fractions. Am I missing something?

The first part is correct. The match function of COMMON_DENOMINATORS returns false when all the terms are fractions and they all have the same denoms or none of the terms are fractions.

I see, it has to have at least one fraction... I obviously need to get more sleep. :P

kevinbarabash · 2017-06-18T21:34:02Z

lib/rule-list.js

+    ? node.args[0].args[1]
+    : node.args[1]
+
+export const isDecimal = (node) => query.isNumber(node) && query.getValue(node) % 1 != 0


Can you add a TODO to move these helper functions to math-nodes?

kevinbarabash · 2017-06-18T21:35:26Z

lib/rule-list.js

+                    : LCM / denom == 1
+                    ? build.number(denom)
+                    : build.mul(build.number(denom),
+                                build.number(LCM / denom)))


These chained ternaries are really hard to understand. Can you rewrite this using if-else statements?

kevinbarabash · 2017-06-18T21:36:37Z

lib/rule-list.js

+                    : build.mul(num,
+                                build.number(LCM / denoms[i])))
+        } else {
+            newDenoms = build.mul(...denoms)


What happens for 2 / x + 3 / x + 4 / y? Can you add a tests case like this? I would expect the result to be 2y / xy + 3y / xy + 4x / xy.

Ah, good catch. Will fix.

kevinbarabash · 2017-06-18T21:39:48Z

lib/rule-list.js

+        } else {
+            newDenoms = build.mul(...denoms)
+            newNumerators = nums.map(
+                (num, i) => build.mul(num, ...denoms.filter(e => e != denoms[i])))


What about situations where there are multiple factors in the denominator to begin with? I think there's an implicit assumption that each fraction as a single factor denominator. Can you add a test case for 2 / (x * x) + 3 / (x * y) + 4 / (y * y)?

I see. In ChangeTypes, MULTIPLYING_POLYNOMIALScomes before COMMON_DENOMINATORS, so we won't see this exact case. We would see, however, 2/ x^2 + 3/ x^1y^1 + 4 / y^2 which we would want to simplify to (2 * y)/ x^2y^1 + (3 * x) / x^2y^1 + (4 * x) / x^2y^1.

A similar test case that we should also consider is:
2 / ((x^2 + 1)^2 * (x + 1)^1) + 3 / ((x^2 + 1)^2 * x)

kevinbarabash · 2017-06-20T01:25:57Z

This looks good except for the failing test. Any idea what's going on there? It looks like the rule returns an empty string.

aliang8 · 2017-06-20T01:48:46Z

This PR is not ready to merge. I am still working on taking care of more edge cases.

kevinbarabash · 2017-06-23T03:52:39Z

@aliang8 is this ready for review now?

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

added rewrite fractional polynomial

41ceb20

kevinbarabash requested changes Jun 16, 2017

View reviewed changes

aliang8 added 10 commits June 18, 2017 12:20

fixed case for number + fraction

a868faf

rm dot file

d55286f

general case

0a7e6aa

refactored negatives logic

25dda64

made output more friendly

5c9274a

few more edge cases

7d8e286

added a convert to fraction rule

e45f4a3

added decimal_to_fraction function

05e2c52

small fix

410afe5

an extra test case

da0efa6

kevinbarabash reviewed Jun 18, 2017

View reviewed changes

change to if statements

e7c80ac

kevinbarabash reviewed Jun 18, 2017

View reviewed changes

still fixing test case

8164020

aliang8 added 5 commits June 20, 2017 00:00

working on simplify_terms

d31d216

almost done with simplify terms

783d092

fix edge cases

6396397

fixing common denominator

1893d87

adding comments and fixed paren issue

43c63ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fractions #50

Fractions #50

aliang8 commented Jun 15, 2017

kevinbarabash Jun 16, 2017

kevinbarabash Jun 16, 2017

aliang8 Jun 16, 2017

kevinbarabash Jun 16, 2017

aliang8 Jun 16, 2017 •

edited

Loading

kevinbarabash Jun 16, 2017

aliang8 Jun 16, 2017

kevinbarabash Jun 18, 2017

aliang8 Jun 18, 2017 •

edited

Loading

kevinbarabash Jun 16, 2017

aliang8 Jun 16, 2017

kevinbarabash Jun 17, 2017

aliang8 Jun 18, 2017 •

edited

Loading

aliang8 Jun 18, 2017

kevinbarabash Jun 18, 2017

aliang8 Jun 18, 2017

kevinbarabash Jun 18, 2017 •

edited

Loading

kevinbarabash Jun 18, 2017

kevinbarabash Jun 18, 2017

aliang8 Jun 18, 2017

kevinbarabash Jun 20, 2017

kevinbarabash Jun 18, 2017

kevinbarabash Jun 18, 2017

kevinbarabash Jun 18, 2017

aliang8 Jun 18, 2017

kevinbarabash Jun 18, 2017

aliang8 Jun 19, 2017 •

edited

Loading

aliang8 Jun 19, 2017

kevinbarabash commented Jun 20, 2017

aliang8 commented Jun 20, 2017

kevinbarabash commented Jun 23, 2017

Fractions #50

Are you sure you want to change the base?

Fractions #50

Conversation

aliang8 commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliang8 Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliang8 Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliang8 Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliang8 Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash commented Jun 20, 2017

aliang8 commented Jun 20, 2017

kevinbarabash commented Jun 23, 2017

aliang8 Jun 16, 2017 •

edited

Loading

aliang8 Jun 18, 2017 •

edited

Loading

aliang8 Jun 18, 2017 •

edited

Loading

kevinbarabash Jun 18, 2017 •

edited

Loading

aliang8 Jun 19, 2017 •

edited

Loading