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

Overhauling SingleListGrader answer structure, validation and grading #261

Closed jolyonb closed 5 years ago

jolyonb commented 5 years ago

This PR updates the answer validation for SingleListGrader. It actually leverages ItemGrader in a neat way that makes everything nicer, in my opinion. Addresses #247 for SingleListGrader (but not ListGrader, which is a whole 'nother beast!).

The code:

grader = SingleListGrader(
    answers=['a', 'b'],
    subgrader=StringGrader()
)
pprint.pprint(grader.config['answers'])

currently produces:

([({u'expect': u'a', u'grade_decimal': 1, u'msg': u'', u'ok': True},),
  ({u'expect': u'b', u'grade_decimal': 1, u'msg': u'', u'ok': True},)],)

whereas in the overhaul, it produces:

({u'expect': [({u'expect': u'a',
                u'grade_decimal': 1,
                u'msg': u'',
                u'ok': True},),
              ({u'expect': u'b',
                u'grade_decimal': 1,
                u'msg': u'',
                u'ok': True},)],
  u'grade_decimal': 1,
  u'msg': u'',
  u'ok': True},)

Hence, we gain the ability to apply grade_decimals and messages at higher levels, so something like

grader = SingleListGrader(
    answers={'expect':['a', 'b'], 'msg':'Ok!},
    subgrader=StringGrader()
)

is accepted.

codecov-io commented 5 years ago

Codecov Report

Merging #261 into master will increase coverage by 1.43%. The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   98.52%   99.96%   +1.43%     
==========================================
  Files          29       29              
  Lines        2508     2517       +9     
==========================================
+ Hits         2471     2516      +45     
+ Misses         37        1      -36
jolyonb commented 5 years ago

This is now working! It was much easier, cleaner and quicker than I had anticipated!

I still need to write tests...

jolyonb commented 5 years ago

@ChristopherChudzicki Ready for review. I think you'll like it. For a major overhaul, this is a whole lot simpler than you may expect! (pun intended ;-)

jolyonb commented 5 years ago

@ChristopherChudzicki Just wondering if you plan to review this, or should I leave it sit a few days and then self-review?

ChristopherChudzicki commented 5 years ago

I’ll try to review it tonight… I started reading through it this morning… But didn’t finish. If I don’t review it by tomorrow morning then I probably won’t for at least a week, so happy to have you do it.

But, I am reasonably confident I can do it tonight

jolyonb commented 5 years ago

Ok, I fixed those two little things. I basically did your suggestion for checking whether or not to add the message, but I had to complicate it somewhat in order to account for nested SingleListGraders... Basically, I added an extra field to the return dictionary that recorded the required information, and then stripped it out in the call method.

codecov-io commented 5 years ago

Codecov Report

Merging #261 into master will increase coverage by 1.47%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #261      +/-   ##
========================================
+ Coverage   98.52%   100%   +1.47%     
========================================
  Files          29     29              
  Lines        2508   2655     +147     
========================================
+ Hits         2471   2655     +184     
+ Misses         37      0      -37
codecov-io commented 5 years ago

Codecov Report

Merging #261 into master will increase coverage by 1.47%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #261      +/-   ##
========================================
+ Coverage   98.52%   100%   +1.47%     
========================================
  Files          29     29              
  Lines        2508   2655     +147     
========================================
+ Hits         2471   2655     +184     
+ Misses         37      0      -37
jolyonb commented 5 years ago

@ChristopherChudzicki Updated and ready for final review.

ChristopherChudzicki commented 5 years ago

Nice resolution to the message issue!