Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing correlated grading #65

Closed
wants to merge 1 commit into from
Closed

Implementing correlated grading #65

wants to merge 1 commit into from

Conversation

jolyonb
Copy link
Collaborator

@jolyonb jolyonb commented Jul 6, 2018

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:

  • Implementation
  • Validation
  • Documentation
  • Course example
  • Tests

Addresses issue #46.

@jolyonb
Copy link
Collaborator Author

jolyonb commented Jul 6, 2018

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

@coveralls
Copy link

Coverage Status

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

@jolyonb
Copy link
Collaborator Author

jolyonb commented Jul 6, 2018

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
Copy link
Collaborator

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

@ChristopherChudzicki ChristopherChudzicki mentioned this pull request Jul 10, 2018
15 tasks
@ChristopherChudzicki
Copy link
Collaborator

ChristopherChudzicki commented Jul 11, 2018

@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 grading: For ItemGraders inside a ListGrader, provide each ItemGrader with the other submitted answer.
  • correlated input For ItemGraders inside a ListGrader, when a student enters their input for one box, allow them to reference their inputs to other boxes.

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
Copy link
Collaborator Author

jolyonb commented Jul 11, 2018

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
Copy link
Collaborator

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

@ChristopherChudzicki
Copy link
Collaborator

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

@jolyonb
Copy link
Collaborator Author

jolyonb commented Jul 12, 2018

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

@jolyonb
Copy link
Collaborator Author

jolyonb commented Jul 12, 2018

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 jolyonb closed this Jul 12, 2018
@jolyonb
Copy link
Collaborator Author

jolyonb commented Jul 12, 2018

Superseded by #80.

@jolyonb jolyonb deleted the correlated branch July 12, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants