mitodl / mitx-grading-library

The MITx Grading Library, a python grading library for edX
https://edge.edx.org/courses/course-v1:MITx+grading-library+examples/
BSD 3-Clause "New" or "Revised" License
14 stars 9 forks source link

is_callable_with_args can be inaccurate with kwargs #69

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

For normal functions, is_callable_with_args calls get_number_of_arguments, which includes positional arguments AND keyword arguments. For numpy ufuncs, is_callable_with_args is probably accurate. (It relies on ufunc.nin, which seems to be the numpy of positional inputs.)

We are primarily using is_callable_with_args for validating the number of positional arguments in student input, so we could change it to be positional-only.

Relevant: https://stackoverflow.com/a/197053/2747370

(In particular, this came up with np.transpose, which is a normal function with signature transpose(array, axes=None), so using np.transpose as a user-function does not work. Of course, a quick fix is to just use lambda x: np.transpose(x) instead.)

jolyonb commented 6 years ago

From my digging earlier, it looked like trying to get this information accurately for anything you could throw at it was fraught at best. I approve of restricting what we're attempting to just positional arguments. Does this require more than just docstring updates?

ChristopherChudzicki commented 6 years ago

I think it would be feasible to have is_callable_with_args check ONLY positional arguments, so that def f(x, something=None): return x would be identified as having 1 argument. (getargspec returns information about the keyword arguments, too. The SO post linked above has good info.) But it doesn't seem crucial; could just update the docstring for now.

jolyonb commented 6 years ago

Works for me. For the use case we have, positional arguments alone are sufficient.

jolyonb commented 6 years ago

Has this been completely addressed by #77?

ChristopherChudzicki commented 6 years ago

77 skirts this issue. But it would be good to at least document that fact that get_number_of_args currently returns the total number of arguments, positional plus keyword.

(Actually, get_number_of_args is currently inconsistent. The nin attribute on numpy.ufunc seems to give positional only, e.g., numpy.sin.nin is 1, even though it accepts a second keyword argument.)

jolyonb commented 6 years ago

I thought get_number_of_args ignored keyword arguments? At least, that was how I was trying to set it up... Sigh. We should fix this...

ChristopherChudzicki commented 6 years ago

@jolyonb See #79.