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

Introduces sampling for numbered variables #49

Closed jolyonb closed 6 years ago

jolyonb commented 6 years ago

This introduces the concept of a numbered variable in FormulaGrader, resolving #41. A separate key, numbered_vars is used to set up numbered variables, which leverage the existing sample_from infrastructure. Here is an example of its use:

grader = FormulaGrader(
    answers='a_{0} + a_{1} + a_{-1}',
    numbered_vars=['a'],
    sample_from={'a': [0,5]}
)

Any integer entered as an index will be accepted by the grader; there is presently no way of restricting it. I don't think this is actually necessary; if a student enters a_{-1} when only positive indices have meaning, they will be (correctly) marked wrong.

There is a little duplication happening in constructing and checking the list of used variables between FormulaGrader and calc.py. That functionality needs to remain in calc.py so it can be used outside of FormulaGrader, but it also needs to be used to construct the numbered variables. I think it's ok.

Note that there is a very small expansion of acceptable variable names in the form of the optional inclusion of a minus sign in tensor notation.

Documentation has been updated, and an example added to the sample course.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0b84ec7ee342affeb820c6d4931bf7b2cfd2c483 on variable_names_with_numbers into ef4a1efd4f212945bf48fa7173193a0ab85cbf3e on master.

ChristopherChudzicki commented 6 years ago

Cool, I thought this was going to be harder!

I think it might be worth testing a few cases (positive/negative/zero), but if you decide to use the numbered_vars_regexp() function above, the doctests could take care of that.

ChristopherChudzicki commented 6 years ago

Oh, and I suspect you were just waiting to finish the review, but changelog and zip!

(At some point, it might be worthwhile merging things like this into a next branch instead of master, so we can have fewer minor releases.)

jolyonb commented 6 years ago

Eh, I'm fine with minor releases every now and again.

Dealing with the case insensitive nature here is a royal PITA. In fact, I'm certain that I've screwed up case-sensitive situations by using a case-insensitive regexp. Sigh.

In fact, I'm so over case-insensitive that I'm going to make a PR right now that exorcises it completely.

jolyonb commented 6 years ago

Ok, see PR #50. After a decision is made on that, I'll rebase and update this appropriately. I think if #50 is accepted, this should be v1.1.

jolyonb commented 6 years ago

I believe this is ready for merge. Please check carefully!