search

google / mathsteps

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

better fix for multiplying like term constant nodes #181

Closed ldworkin closed 7 years ago

ldworkin commented 7 years ago

https://github.com/socraticorg/mathsteps/pull/180#discussion_r125961158

evykassirer commented 7 years ago

thanks for updating this!

also can you add a test somewhere that failed before and doesn't anymore, so it stays right in the future? tests in this repo are usually fairly quick to add, should just be one line of asciimath input/out example.

ldworkin commented 7 years ago

What would that test look like @evykassirer ? Cause the simplification was still "correct" before, as in the final answer was fine, but the steps it took to get there were excessive.

EDIT: Does this work? https://github.com/socraticorg/mathsteps/pull/181/commits/40e37af7da06791d2e7464819b98aaf5fa93d917 i.e. does this mean it has to happen in exactly one step?

evykassirer commented 7 years ago

Oh sorry - true. I'd recommend using TestUtil.testSubsteps (if it had substeps before)

I think there might be one for just testing steps (not substeps) already - and if there's not it'd be nice to add one at some point - I think a lot of the bugs these days are bad steps even if it gets the right answer (if that's too low priority for you I can add that test myself this weekend!)

evykassirer commented 7 years ago

oo maybe simplifyExpression/oneStep.test.js would work here

ldworkin commented 7 years ago

Cool! Added that. And I double checked that it would have failed before :)

aliang8 commented 7 years ago

Thanks! This looks cleaner.

evykassirer commented 7 years ago

sweet! glad that was easy