Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.

Comments

distribute fractions normally#207

Merged
ldworkin merged 3 commits intomasterfrom
ld_distribute_fractions
Jul 19, 2017
Merged

distribute fractions normally#207
ldworkin merged 3 commits intomasterfrom
ld_distribute_fractions

Conversation

@ldworkin
Copy link
Contributor

@ldworkin ldworkin commented Jul 18, 2017

Not sure why we had special logic for fractions before.

Previously:
4(5/4 + x) => (5 * 4)/4 + x * 4

Now:
4(5/4 + x) => 4 * (5/4) + 4 * x

@evykassirer
Copy link
Contributor

omg lol I think I know why this was the case - remember how the cancelling didn't work that well before? I think this was to get around that (not totally sure but that's my guess for now)

['(2x + x^2) * (3x^2 / (x^2 -4) + 4x^2)',
['((3x^2 * (2x + x^2)) / (x^2 - 4) + 4x^2 * (2x + x^2))',
'((3x^2 * (2x + x^2)) / (x^2 - 4) + (8x^3 + 4x^4))']
['(x - 4) * ((3x^2)/ (x^2 - 4) + 1)',
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why you changed these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the old ones were failing -- the answers were equivalent but just came out looking different due to the change. I tried fixing them, but they were pretty complicated and it was hard to follow, so I replaced them with some slightly simpler ones, so it's easier to see what's going on.

Copy link
Contributor

@evykassirer evykassirer left a comment

Choose a reason for hiding this comment

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

👍 legit

@ldworkin
Copy link
Contributor Author

Make sense re: cancelling ... I think?! Though the change I made to the cancelling code was to make it work for parens, and there are no parens here ... anyway hopefully this doesn't break anything terribly! I tested a bunch and Gen will do so more.

@ldworkin ldworkin merged commit a21240d into master Jul 19, 2017
@ldworkin ldworkin deleted the ld_distribute_fractions branch July 19, 2017 12:19
ldworkin added a commit that referenced this pull request Jul 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants