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

handle_node breaks when functions have arguments of different types #104

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

A very minimal example:

>>> import numpy as np
>>> np.isinf( np.array([1, 2, 3]), 5 )
Traceback (most recent call last):
TypeError: return arrays must be of ArrayType

I don't completely understand the issue, but apparently np.isinf cannot handle a list where some members are arrays and some members are not. Same issue with np.isnan.

This shows up for us in calc.py, where the self.actions['arguments'] outputs a list of function arguments, some of which might be arrays, some of which might be numbers.

Here's a full example that produces the error:

import numpy as np
from mitxgraders import MatrixGrader, RealVectors

def rot(vec, axis, angle):
    """
    Rotate vec by angle around axis. Implemented by Euler-Rodrigues formula:
    https://en.wikipedia.org/wiki/Euler%E2%80%93Rodrigues_formula
    """
    unit_axis = axis/np.linalg.norm(axis)
    a = np.cos(angle/2)
    omega = unit_axis * np.sin(angle/2)
    crossed = np.cross(omega, vec)

    return vec + 2*a*crossed + 2*np.cross(omega, crossed)

grader = MatrixGrader(
    answers='rot(a, [0, 0, 1], theta)',
    variables=['a', 'theta'],
    sample_from={
        'a': RealVectors(shape=3),
    },
    user_functions={
        'rot': rot
  },
  debug=True
)

(Discovered while writing documentation for specify_domain)

jolyonb commented 6 years ago

Is this a simple matter of mapping is_inf over a list? Where exactly does the error occur?

ChristopherChudzicki commented 6 years ago

It happens here:

https://github.com/mitodl/mitx-grading-library/blob/master/mitxgraders/helpers/calc/calc.py#L589 https://github.com/mitodl/mitx-grading-library/blob/master/mitxgraders/helpers/calc/calc.py#L592

Is this a simple matter of mapping is_inf over a list?

Yes, I think so. And probably wrap results in a list if it isn't one already. (I believe the only case when results should be a list is with self.actions['arguments'].)

But I was going to remove IdentityMultiple before dealing with this, since I think that will make some of the comments simpler (e.g., https://github.com/mitodl/mitx-grading-library/blob/master/mitxgraders/helpers/calc/calc.py#L583 will say something like "Float, MathArray, or List (in case of function arguments)".