partial credit for autograde test cells

[kcranston]

Closes #974 . When grading, look for output from an autograde test cell that is a single value that can be cast to a floating point value greater than 0. Return min(output, max_points), i.e. if the cell returns more than the max_points, simply return max_points.

Thinking of this as version 1.0. In subsequent versions, planning to allow an autograde cell to return formatted json output (which can include scores, messages for students, tracebacks from errors, etc).

Have not added docs yet - looking for feedback on the implementation first, then I can modify docs either in this PR or a subsequent.

[kcranston]

Looks like I've got a failing test. Let me investigate....

[kcranston]

Still one test failing (test_nbgrader_feedback.py::TestNbGraderFeedback::test_force_f), but only in the appveyor python 3.5 environment, not the 2.7 environment and not locally for me. Will investigate, but any feeback most welcome.

[jhamrick]

This looks really great, thanks! My comments are mainly just having to do with when to raise errors versus not---otherwise I am very happy with this.

[jhamrick]

btw, I think the test failure is probably spurious, I have restarted the tests to see.

[kcranston]

Thanks for the comments! - nothing objectionable from my perspective. Will make the modifications in the next day or two.

[kcranston]

@jhamrick - can you point me to an example for for how you would have code in utils.py return a warning to the calling app? I don't quite understand how the logging works. Do I need to create a LoggingConfigurable class in utils? (similar to in validator.py)?

[jhamrick]

@kcranston I think the easiest way to do it would be to have the get_partial_grade and determine_grade functions in utils.py accept a log argument (default=None, in which case don't log anything), and then pass in self.log from the calling app.

[jhamrick]

Probably better to keep this as it was, in case the score is greater than the max score that also shouldn't count as failed:

Suggested change

	elif score == max_score:
	elif score >= max_score:

[kcranston]

I can change that, but then also need to update get_partial_grade, which currently returns a ValueError when score > max_score. Should I revert that back to a warning again?

[jhamrick]

Thanks! I just have the one comment about the comparison between the score and max score, otherwise I think this is good to merge.

[jhamrick]

Oh, I see, sorry I missed that. No, in that case I think it should be fine. I just want to test this doesn't cause problems when assigning extra credit which I'll try to do tonight and then that works ok I will go ahead and merge. Thanks a lot for working on this!

…

On Wed, Jun 19, 2019 at 8:06 AM Karen Cranston ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nbgrader/validator.py <#1090 (comment)>: > # it's a markdown cell, so we can't do anything if score is None: pass - elif score >= max_score: + elif score == max_score: I can change that, but then also need to update get_partial_grade, which currently returns a ValueError when score > max_score. Should I revert that back to a warning again? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1090?email_source=notifications&email_token=AAAUL5AGZOIVSDLZGDIEK7LP3IOL5A5CNFSM4HPRK7MKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB376R4Y#discussion_r295261254>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAUL5ECHUYAIWNRGX5S7TDP3IOL5ANCNFSM4HPRK7MA> .

[kcranston]

Thanks, @jhamrick ! I'll work on updating the docs, along with an example notebook (separate PR).

[lwasser]

hey @kcranston @jhamrick curious what the time associated with getting this merged is?
thank you!!

[kcranston]

Pinging again. @lwasser and I are writing notebooks that use this functionality and planning to use in a class starting next week. Is it possible to have this merged this week?

[lwasser]

hey @jhamrick i know you are super busy. we do need to use this functionality in our class and it starts next week. what do you need from us to get this merged at this point? we are installing from conda-forge so it would be ideal to get things merged and to bump a patch (IF you are comfortable with that) this week to support our environment setup and such. please just let us know what you need.