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

Implementing correlated grading #65

Closed jolyonb closed 6 years ago

jolyonb commented 6 years ago

This PR adds a dependent_input config variable to each ItemGrader class that specifies which inputs are required to perform the check for that ItemGrader. When a lowest-level ListGrader has ordered input with individual subgraders, it reads the dependent_input list from each subgrader. Those with empty lists, it evaluates. Those that have dependencies, it passes through to the check function as a keyword argument.

I've implemented dependent_input for FormulaGrader only (and NumericalGrader by subclassing). For FormulaGrader, the dependencies are computed after variables have been sampled. The dependencies are labelled input_n, and added to the variables list before computing any comparer_params. This means that input_n dependencies can be used in the answer strings for such graders without needing a custom comparer. The input_n variables are scrubbed from the variables list before the student input is checked, so students cannot take advantage of it.

This was actually surprisingly straightforward to implement. So far however, this is just the raw implementation. This still needs the following:

Addresses issue #46.

jolyonb commented 6 years ago

Alas, I'm out of time to work on the library now -- other work beckons! Can I leave this in your capable hands?

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-2.2%) to 97.84% when pulling 7900b81c5352144c675a9e3cd97fe8afc6c97e3a on correlated into 06b7366b4d137d2a311528511dbbf62e661658b1 on master.

jolyonb commented 6 years ago

I should add - here's an example.

grader = ListGrader(
    answers=["1", "1 + input_1"],
    ordered=True,
    subgraders=[
        FormulaGrader(),
        FormulaGrader(dependent_input=[1])
    ],
    debug=True
)
ChristopherChudzicki commented 6 years ago

Happy to pick this up after at least an initial PR for the matrix stuff.

ChristopherChudzicki commented 6 years ago

@jolyon This is really cool works great, even with nested ListGraders. I think there might be a way to do it more simply with a simpler interface for authors.

I've been thinking about correlated grading and correlated inputs for ListGrader, and it strikes me that they are actually two separate issues:

Correlated input sounds interesting but hard. See further comments below.

Correlated Grading

I think we can get a lot of mileage out of correlated grading and implement correlated grading more simply, without checking for circular dependencies or requiring subgraders. For example:

grader = ListGrader(
    answers=['a', 'input_1^2', '(input_1 + input_2)^2'],
    subgraders=FormulaGrader(variables=['a'])
)

grader(0, ['a', '2*a', '3*a']) # This input is not correct!

In this example, when the ListGrader calls FormulaGrader's check method, it would pass an additional keyword argument every time:

sibling_values = ['a', '2*a', '3*a']
# FormulaGrader would be responsible for turning this into 
# {'input_1': 'a', 'input_2': '2*a', 'input_3': '3*a'}

Note in particular that circular references are not a problem. For example:

grader = ListGrader(
    answers=['input_2', 'input_1'],
    subgraders=FormulaGrader(variables=['a'])
)
grader(0, ['a', 'a'])['ok'] # True !
grader(0, ['10*a', '7*a + 3*a'])['ok'] # True !
grader(0, ['', ''])['ok'] # Empty strings are evaluated as 'nan', so I don't know what would happen here...

Well, ok. Circular references would allow authors to do stupid things like answers=['input_2', 'input_1 + 1'], but I don't think we should worry about that.

One nice thing about this approach is that it would let you use a single FormulaGrader() for your whole list, so you wouldn't need to re-specify variables, etc.

Correlated Input

Allowing students to reference their own inputs sounds harder and would definitely require sorting dependencies. I think it also only makes sense for FormulaGrader. I would suggest implementing it through DependentSampler. Soemthing like:

# Problem: List the 2nd through 4th terms in a geometric sequence with common ratio r and initial value a0 = 2.
grader = ListGrader(
  # answers are named 'a2', 'a3', 'a4'
  answers=['2r','2r^2', '2r^3'],
  subgraders=FormulaGrader(
    variables=['a2','a3','a4', 'r'],
    sample_from={
      'a2': DependentSample(depends=['input_1'], formula='input_1'),
      'a3': DependentSample(depends=['input_2'], formula='input_2')
    }
  )
)

This would require some changes to how DependentSampler evaluates.

Overall, I think correlated inputs are not worth it without a compelling reason.

(BTW: If we ever do need to change how DependentSampler works, or sort dependencies in some other area, I think we should use a package like toposort to do this for us.)

jolyonb commented 6 years ago

Indeed, passing all siblings and exposing these as variables sounds like a clean way to do it. You can get away without doing the circular reference check, but note in my example that it only exists to ensure that the author hasn't set one up - I could have just evaluated them all anyway. Up to you.

I'm not interested in correlated input at this stage. Students can do their own damn calculations ;-)

ChristopherChudzicki commented 6 years ago

OK. I'm going out for a bit, but I'll refactor it that way later.

ChristopherChudzicki commented 6 years ago

@jolyonb Travis seems not to care if coverage decreases anymore. Did you change that on purpose?

jolyonb commented 6 years ago

That's a coveralls thing. I don't know why it isn't complaining...

jolyonb commented 6 years ago

What I was thinking about for validation was requiring ordered and a list of subgraders in order to be using the dependent_input key.

I think at the end of the day, issue 1 is ok -- it's not like during config we validate that the expression given by the instructor can even evaluate.

Issue 2 -- I agree that we shouldn't worry about it. It's equivalent to the instructor putting a wrong answer in anyway.

jolyonb commented 6 years ago

Superseded by #80.