Implementing multi-argument functions and fixing exponentiation issue

[jolyonb]

Addresses issues #42 and #43 by rewriting calc.py to fix exponentiation problem (introduced "negation" operation) and allow multi-argument functions (introduced "arguments" operation). Also did general tidying of pyparsing script and some refactoring of calc.py, along with a little linting. Large changes to your get_number_of_args function were necessary to pull this off.

[jolyonb]

I'm quite proud of this one. Let me know what you think!

[coveralls]

Coverage remained the same at 100.0% when pulling fcd152a on multivar into adaab0c on master.

[ChristopherChudzicki]

This looks great! I like your restructuring of the parser and evaluator! And its still pretty fast :).

I made a few comments, at least one of which is somewhat substantial.

[ChristopherChudzicki]

Should this (and the next few) be eval_negation? (Or, eval_sign if you take the suggestion above.)

[jolyonb]

Indeed it should. Amusing that eval_power satisfies all these tests too! (I could just point it to eval_power, but I think that's bad practice)

[ChristopherChudzicki]

Example	Expression	master parses?	multivar parses?
A	1+2	yes	yes
B	1+-2	yes	yes
C	1-+2	yes	no
D	1++2	yes^1	no
E	1+++2	no	no
F	---2	no	yes
G	--+2	no	no

footnote 1: '++2' behaves similarly, both are parsed by master roughly as:

<sum>
  <number>1</number>
  +
  <number>
   +1 
  </number>
</sum>

Examples C through G are certainly a bit contrived, but I don't think they're too ridiculus.

I'm actually pretty confused about why the ZeroOrMore in line #382 doesnt' permit 1+++2 (on either this branch or master).

Suggestion: Change negation to sign or unary_sign, and change eval_negation to eal_sign. (I can commit my local version of this if you want, or you can write it.)

[jolyonb]

So, I deliberately treated + and - differently here, basing things on how google parses math input. I don't want to accept + as part of a number, like 1-+2 would require. It's ok for - to be part of a number, which is why 1-------2 works - you just keep on adding brackets around the 2 with an ever-increasing number of minus signs.

I always thought it was strange that the old parser allowed for two pluses in a row, but not three, also. The reason is that it allowed for a single plus to be a part of a number, and then a second plus to be a part of a sum.

I think we should discourage inputs like your D. I also don't like C, E and G for the same reason. I'm happy with B, and I'm willing to tolerate F.

Note that like 382 contains ZeroOrMore(plus_minus + product), so you must have an expression after a +/- sign.

[ChristopherChudzicki]

I don't want to accept + as part of a number

Still thinking about what you said, but to be clear my suggestion was not to modify the number group, but:

# replace negation with this:
sign = Group(ZeroOrMore(plus_minus) + power)("sign")

# and later, replace eval_number with
def eval_sign(self, parse_result): 
        num_negations = len([char for char in parse_result[:-1] if char == '-' ])
        num = parse_result[-1]
        return num * (-1)**(num_negations)

It just seems unncessarily arbitrary to me that ---1 is allowed but +++1 isn't. (Not that either is common, presumably).

[jolyonb]

Yup, I understood the suggestion.

Do you agree with me that we shouldn't allow C/D/E/G and we need to allow B?

I'd be willing to get rid of F.

To add another case, I think 1--2 needs to be allowed, but 1---2 can probably go.

I think these can be accomplished by changing ZeroOrMore to Optional in the negation statement.

[ChristopherChudzicki]

Yes, I agree those are bad. Thinking about it more—I like rejecting them as unparseable: I think it would be easy to miscount the number of signs.

I agree Optional is a good choice.

But really, I guess it doesn't matter. I just checked some 2015 data, and in 400,000 submissions I found about 25 +-, 10 -+, and 7 ++. (Not all of the 400,000 were FormulaResponse submissions, but I assume quite a lot were. So...rare.)

[ChristopherChudzicki]

A bunch of stuff in test_base.py testing stuff from mitxgraders.helpers. Maybe we should just make test/helpers/test_calc.py and test/helpers/test_validatorfuncs.py?

[jolyonb]

I've refactored it out as suggested.

[ChristopherChudzicki]

sweet :)

[jolyonb]

I know, right? I was quite happy not to have to construct it in terms of more low-level functionality. And it will make doing vectors/matrices quite straightforward, too!

[jolyonb]

Ok, made some minor modifications, and responded to your more major point.

I'm pretty happy that that's all you were able to find, given my 4:30 am coding!