generalizing polynomial terms

[ldworkin]

So the motivating issue was to get this working: sqrt(x) + sqrt(x) = 2*sqrt(x), or 3*sqrt(11) - 2*sqrt(11), etc.

After playing around with a bunch of different approaches, I came to the realization that what I really wanted was for the code to treat the sqrt(...) part like the symbol part of a polynomial. Then, everything else would work as expected.

So my solution was to generalize the PolynomialTerm class into a TermWithCoefficient class, which represents any term with a 1) coefficient, 2) "base" (e.g. symbol or nthroot), and 3) exponent. Then PolynomialTerm and my new NthRootTerm can both subclass from this.

In the future, we could also create subclasses for, say, binomials, and then e.g. 4(x+1) + 3(x+1) would become (4+3)(x+1), instead of distributing left and right as it does now (I think this came up in another issue).

Only iffy thing is my terminology. "Coefficient" is maybe the wrong word here; I'm not sure.

[ldworkin]

Just fyi this isn't quite ready for review -- need to add tests/comments and will write a description explaining what the PR does. It's involved, hehe.

@aliang8 This might affect some of what you're doing with square roots, so maybe take a quick look, or we can discuss it in person tomorrow!

[aliang8]

Okay, will do just that. :D

[ldworkin]

This is ready for review now, I believe! It's a bit on the larger side, but the idea is simple. See the PR description for some context. (Evy feel free to take your time reviewing this too, since it's a little more involved.)

[evykassirer]

I'm gonna start a big change to my sib's festival website tonight, so will leave this for now. Maybe Anthony can review it first to take a first crack at things? And I'll take a look after :)

[aliang8]

Great stuff Lili!!! I like the new change to termsWithCoefficient. The name seems a bit clunky, but that shouldn't be a big issue. I left a few minor comments regarding excess code and files. But I think you've got the general idea and seem to be very comfortable working on mathsteps. You're such a quick learner Lili!

This is ready for your review Evy! 👍 💯 🥇

[evykassirer]

cool stuff! I'm going to swing tonight (no more time tonight) and just spent my review time on factoring (lol that PR)

Is this stuff urgent? I can prioritize it tomorrow if it's blocking, or can take my time Friday night/over the weekend :)

[ldworkin]

Hmm, not urgent, weekend is probably fine. But if we start getting too backlogged, we might need to think of a better system for all this. I don't know if we'll be able to sustain the same level of detailed review that we've been doing so far ...

[evykassirer]

I have a feeling that as we go, review will get less 'detailed' because we'll all know more about how each other thinks about the code and you'll all be handling more edge cases (or at least all the ones I can think of) and stuff

but we can reevaluate next week if we feel it's not going at that pace enough :) I am a bit worried about putting buggy or unclear code into an open source repo though - makes it harder for people to come in from outside and work on it

[evykassirer]

this is first priority for me tomorrow (unless you have something else for me to look at you'd like to me to do first)! might even get to it before work :)

[ldworkin]

Thanks but it's really fine! I don't want to stress you out! Just trying to think about the right way to approach this stuff, esp. with a lot of pressure from product team to be working quickly over the next few weeks.

[ldworkin]

Yay, thanks for the review Evy! Sorry if I scared you by saying it was a lot of code, hahaha.

I think I addressed all your comments 😄

[evykassirer]

also let me know what you decide with this

[ldworkin]

Great! I opened an issue here: https://github.com/socraticorg/mathsteps/issues/198 instead of adding to Asana, since it doesn't seem super high priority (not breaking anything, afaik).