search

google / mathsteps

Step by step math solutions for everyone
https://socratic.org
Apache License 2.0
2.12k stars 274 forks source link

Factoring fix #184

Closed aliang8 closed 7 years ago

aliang8 commented 7 years ago

fixes: https://cl.ly/0n2k3P1v1K3m vs https://cl.ly/3P0o0m33202m

and https://s3.amazonaws.com/socratic-ocr-uploads/prod/2fe54324-0e6e-4af6-860f-b22ba8ae4d11/raw.jpeg

ldworkin commented 7 years ago

@evykassirer Curious if you could elaborate on why you think this is hacky, and what you think a better approach would be? In general I think we might have to go with hacky solutions for now given how crunched for time we are, but I do want to understand what the "right" way to do it would be!

evykassirer commented 7 years ago

@ldworkin hacky because of all the edge cases it misses - it will probably throw a lot of errors (for reasons I pointed out in comments). I think you could maybe cover up those errors with a try catch (where the catch catches more), but that'd feel gross to me

ldworkin commented 7 years ago

@evykassirer Gotcha, thanks! Again I think we might have to settle for hacky/gross solutions right now, but we can still try to add explanatory comments and clean up the code as much as possible. @aliang8 can you add comments including several examples of cases that are supported and that aren't? Right now it's unclear to me what is covered and what isn't.

aelnaiem commented 7 years ago

Don't mean to slow this down so leaving this as a future point, but I feel that this logic should live in the factoring module, so maybe a future TODO would be to move it into it's specific module.

But it would probably be straight-forward to move it now

aliang8 commented 7 years ago

Hey Ahmed! I don't think this logic can go into the factoring module. We have to first recognize that we are dealing with an equation that is = 0 before we can find the roots. Can you elaborate a little more on why you think it should go into the factoring module?

ldworkin commented 7 years ago

@evykassirer is this good to approve?

ldworkin commented 7 years ago

No worries if review takes a long time, especially for something as complicated as this. I'll read this over carefully today, and then maybe @aliang8 we could pair on cleaning it up.

ldworkin commented 7 years ago

Ok, I just looked through this and got comfy with it. Left a few comments that should be easy to address. Once you fix those, hopefully @evykassirer can take one more quick look and then we can merge.

I'm adding an Asana task to come back and address the issue where it only returns one of the roots. (e.g. (x-2)(x+2) = 0 will only give x=2). That concerns me, because it's kinda giving the wrong answer, so I'll try to fix soon! But I think that can go in another PR.

aliang8 commented 7 years ago

Merging !