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

Pass siblings #80

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

This PR addresses #46 by passing a siblings keyword argument to the check function of subgraders in a ListGrader. This parameter is only available when ordered=true.

Currently only FormulaGrader makes any use of siblings in its check function. Example:

grader=ListGrader(
    answers=['sibling_3 + 1', 'sibling_1^2', 'x'],
    subgraders=FormulaGrader(variables=['x']),
    ordered=True
)
grader(None, ['x + 1', 'x^2 + 2*x + 1', 'x'])

Some notes:

ChristopherChudzicki commented 6 years ago

@jolyonb Cool, I'll fix those typos!

One of the things on your checklist for #65 was validation. Two comments about this:

  1. I suspect the most common configuration error would be something like:

    grader = listGrader(
    answers=['1', 'sibling_1 + 1'],
    subgraders=FormulaGrader()
    ) # instantiated OK
    grader(None, ['1', '2']) # throws InvalidInput: sibling_1 undefined

    The issue is that the author forgot to declare the ListGrader as being ordered. FormulaGrader validates itself without any knowledge of sibling variables or the ListGrader's answers, so the error doesn't arise until check time.

    • This is sort of present in your original implementation (same error if author tries to use input_k variables without declaring dependent_inputs). You avoided number 2 by checking for cyclic dependencies in dependent_inputs.
  2. Authors can set up ridiculous things like

    grader = ListGrader(
    answers=['sibling_1 + 1'],
    subgraders=FormulaGrader(),
    ordered=True
    )

    Nothing would ever be marked correct, since the answer cannot be 1 bigger than itself.

    • I am not very worried about this issue.

Mostly: I think avoiding the extra key (dependent_inputs) and allowing reuse of the subgrader is worth the trouble of these less-than-helpful error messages.

I'm also not super worried about it for what I suspect will be an uncommonly used feature.

A hacky but fast resolution to the first issue would be to raise a special error if the undefined variable contains 'siblings'. Students might see this, but why would they be entering 'sibling_k' as a variable name....

Anyway, I think the PR is fine, but I wanted you to be aware of those issues.

ChristopherChudzicki commented 6 years ago

@jolyonb Typos fixed. I need to figure out a good way to spellcheck my code in Atom... sorry about that :)

jolyonb commented 6 years ago

Ah, I responded to your comment in #65...