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

#76 - Better break-up fraction. #158

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@eltonlaw

eltonlaw commented Apr 21, 2017

fixes #76

This is only solves the initial case posed of (2x+3)/(2x+2). The output is now
(2x+2)/(2x+2) + (1)/(2x+2).

eltonlaw added some commits Apr 21, 2017

Issue #76 - Better break-up fraction.
This is only solves the initial case posed of (2x+3)/(2x+2). The output is now
(2x+2)/(2x+2) + (1)/(2x+2).
@evykassirer

looking good! thanks for working on this, a nice small change that'll add lots of teaching power to mathsteps :D

I left some comments for stuff to think about to make this better

Show outdated Hide outdated lib/checks/canFindDenominatorInNumerator.js
Show outdated Hide outdated lib/checks/canFindDenominatorInNumerator.js
Show outdated Hide outdated lib/checks/canFindDenominatorInNumerator.js
Show outdated Hide outdated lib/checks/canFindDenominatorInNumerator.js
Show outdated Hide outdated lib/simplifyExpression/breakUpNumeratorSearch/index.js
Show outdated Hide outdated lib/simplifyExpression/breakUpNumeratorSearch/index.js
if (denominatorParenRemoved) {
denominator = Node.Creator.parenthesis(denominator);
}
}

This comment has been minimized.

@evykassirer

evykassirer Apr 21, 2017

Member

I think it'd be best if we can break this up into two steps. the step you added, and the break up fraction that comes after (and then maybe we can group in the cancelling that comes after). and then we'd have substeps for breaking up the fraction.

what do you think? (let me know if you want me to explain more what this means)

@evykassirer

evykassirer Apr 21, 2017

Member

I think it'd be best if we can break this up into two steps. the step you added, and the break up fraction that comes after (and then maybe we can group in the cancelling that comes after). and then we'd have substeps for breaking up the fraction.

what do you think? (let me know if you want me to explain more what this means)

Show outdated Hide outdated test/checks/checks.test.js
Show outdated Hide outdated ...simplifyExpression/breakUpNumeratorSearch/breakUpNumeratorSearch.test.js

eltonlaw added some commits May 4, 2017

Issue #76 - Better break-up fraction
Almost completely covers all cases of (ax^1 + b) / (cx^1 + d). The only
current holes in this function are for cases where 1) Constant and
polynomial term are not ordered 2) When b is equal to 0

 'checks/canFindDenonimatorInNumerator': The checks are more specific than
before.

'breakUpNumeratorSearch/index.js': Included a multiplier variable to see
how much times the denominator is in the numerator. Instead of using
numerator.args[1] it's now the args[num_n-1] to get the last index, this
way as long as it's sorted we'll always get the constant.

'checks/checks.test.js': More test cases, included falses

'breakUpNumeratorSearch/breakUpNumeratorSearch.test.js': More test cases
@evykassirer

sorry for the delay - was at a conference last weekend! I'll try to be more quick back and forth with you now

I think we're gonna have to think a bit harder about how to get this working properly (unless I missed something) so let me know if you want help thinking about it!

@eltonlaw

This comment has been minimized.

Show comment
Hide comment
@eltonlaw

eltonlaw May 15, 2017

I think I incorporated all your feedback @evykassirer, as things are now is this okay to get merged?

eltonlaw commented May 15, 2017

I think I incorporated all your feedback @evykassirer, as things are now is this okay to get merged?

@evykassirer

sorry - had to leave some more comments

I think one thing that'd help with readability a lot (and help me review easier hehe) is make a clear list of the conditions that must be true for it to return true at the top of the function declaration - exactly what the things you're going to check for are. Having them all together like that will make it easier to see what you're describing - what do you think?

@evykassirer

This comment has been minimized.

Show comment
Hide comment
@evykassirer

evykassirer May 18, 2017

Member

I'm at GoogleIO this week and can't take much time to review your code - but I replied to some comments and will actually look at the diff on the weekend

thanks for your patience! soon these rules will be easier to write hopefully - someone's working on a cleaner way to describe them right now :) (if you want to see more, check out https://github.com/semantic-math/math-rules)

Member

evykassirer commented May 18, 2017

I'm at GoogleIO this week and can't take much time to review your code - but I replied to some comments and will actually look at the diff on the weekend

thanks for your patience! soon these rules will be easier to write hopefully - someone's working on a cleaner way to describe them right now :) (if you want to see more, check out https://github.com/semantic-math/math-rules)

@evykassirer

This comment has been minimized.

Show comment
Hide comment
@evykassirer

evykassirer May 22, 2017

Member

much cleaner :) thanks

Member

evykassirer commented on lib/checks/canFindDenominatorInNumerator.js in 308a90c May 22, 2017

much cleaner :) thanks

['(2x + 3)/(2x + 2)', true],
['(2x+3)/(2x)', false], // Normal breakup function already solves this
['(2x)/(2x + 2)', true],
['(5x + 3)/(4)', false], // Normal breakup function already solves this

This comment has been minimized.

@evykassirer

evykassirer May 22, 2017

Member

these comments are super helpful!

@evykassirer

evykassirer May 22, 2017

Member

these comments are super helpful!

@aliang8

Hey Elton!
Good job on this pull request! Apologies for taking so long to submit my own code review. Thanks for taking the time to clean up the code, write helpful comments and add test cases. One quick change and this should be good for merging. I see you commented out the test case 2x / (2x + 3). Why is that? Isn't it suppose to pass? Like Evy said, if it passes uncomment it and if not, add a TODO to handle the case in a future PR. This is a great start to fraction manipulation. Hopefully you can continue to add more support for handling fractions. You could probably start by finding a way to rearrange the terms in a polynomial based on exponent. This would allow you to fix the case you skipped. Anyways, good job! Push the small change and we should be good to go.

@eltonlaw

This comment has been minimized.

Show comment
Hide comment
@eltonlaw

eltonlaw May 22, 2017

Hey @aliang8 and @evykassirer, I think the problem with 2x / (2x +3) is the following line in breakUpNumeratorSearch/index.js:

if (!Node.Type.isOperator(numerator) || numerator.op !== '+') {
    return Node.Status.noChange(node);
}

the second part numerator.op !== '+' evaluates to true so the no change node is returned. I was thinking about adding a part before this where it adds an op to add a constant node '0' to the numerator in the case of a single polynomial numerator. But idk, it might end up really fragile. What do you think we should do?

eltonlaw commented May 22, 2017

Hey @aliang8 and @evykassirer, I think the problem with 2x / (2x +3) is the following line in breakUpNumeratorSearch/index.js:

if (!Node.Type.isOperator(numerator) || numerator.op !== '+') {
    return Node.Status.noChange(node);
}

the second part numerator.op !== '+' evaluates to true so the no change node is returned. I was thinking about adding a part before this where it adds an op to add a constant node '0' to the numerator in the case of a single polynomial numerator. But idk, it might end up really fragile. What do you think we should do?

@aliang8

This comment has been minimized.

Show comment
Hide comment
@aliang8

aliang8 May 22, 2017

Contributor

Ah good call, I see where you're referring to. Maybe you could add in a check to see if the numerator is a single polynomial term or if it is just a constant and handle it accordingly inside that if statement. By doing this, we can restrict to only the cases that we handle.

So in practice that if statement could be: if ((!isOperator(node) || (numerator.op != '+')) && notSinglePolynomialTerm(node)) {return} It seems a bit clunky as of now, but hopefully you could think of a better way to handle this.

Contributor

aliang8 commented May 22, 2017

Ah good call, I see where you're referring to. Maybe you could add in a check to see if the numerator is a single polynomial term or if it is just a constant and handle it accordingly inside that if statement. By doing this, we can restrict to only the cases that we handle.

So in practice that if statement could be: if ((!isOperator(node) || (numerator.op != '+')) && notSinglePolynomialTerm(node)) {return} It seems a bit clunky as of now, but hopefully you could think of a better way to handle this.

@evykassirer

This comment has been minimized.

Show comment
Hide comment
@evykassirer

evykassirer May 23, 2017

Member

What about if you check your own conditions first, then check
if (!Node.Type.isOperator(numerator) || numerator.op !== '+') {
if your conditions don't pass?

Member

evykassirer commented May 23, 2017

What about if you check your own conditions first, then check
if (!Node.Type.isOperator(numerator) || numerator.op !== '+') {
if your conditions don't pass?

@eltonlaw

This comment has been minimized.

Show comment
Hide comment
@eltonlaw

eltonlaw May 27, 2017

hey @evykassirer @aliang8, I've been trying to debug this but I can't seem to identify the problem. I've been poking around but I just can't seem to make sense of what's going on. When I try to incorporate the new checks everything seems to blow up. Sometimes tests from addConstantFractions give out errors, other times a bunch of tests in solveEquation fail. I am baffled. Do you guys want to check it out?

In the meantime, I just commented it out and left a TODO.

eltonlaw commented May 27, 2017

hey @evykassirer @aliang8, I've been trying to debug this but I can't seem to identify the problem. I've been poking around but I just can't seem to make sense of what's going on. When I try to incorporate the new checks everything seems to blow up. Sometimes tests from addConstantFractions give out errors, other times a bunch of tests in solveEquation fail. I am baffled. Do you guys want to check it out?

In the meantime, I just commented it out and left a TODO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment