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

Grading incompatible types #94

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

Provides configuration keys to customize what should happen when a student submits an answer to MatrixGrader that has the wrong shape.

ChristopherChudzicki commented 6 years ago

@jolyonb This is ready for review. I will address any changes you think should happen to this, but other than that I probably won't do anything until July 28th. (In particular, let me know if you think different default settings would be better.)

jolyonb commented 6 years ago

Apart from some spelling/grammar, the implementation in default_equality_comparer is perfectly fine. However, can we take the try/except out of default_equality_comparer and wrap it around the call to the comparer, so that other comparers benefit from the same error handling?

Edit: This may mean that we need to move the call to comparer in FormulaGrader into its own function so that it can be subclassed by MatrixGrader.

ChristopherChudzicki commented 6 years ago

Regarding

However, can we take the try/except out of default_equality_comparer and wrap it around the call to the comparer, so that other comparers benefit from the same error handling?

I think you're right that we need a more generic solution here, but I'm not sure that this will work in general. In order to make appropriate error messages, we need to know what shape the answer should be. In the first version of this PR, the expected shape was determined from comparer_params[0] which, for the default comparer, is the actual answer. But for a general comparer, the shape of comparer_params[0] might not be the shape of the answer. (For example, for a comparer like is_eigenvector_of.)

So instead I moved the shape validation to a separate function, and this function is provided to the comparer function as part of utils.

Also:

The original version of this PR had a number of issues that I've hopefully now resolved.

jolyonb commented 6 years ago

Looking much better. Few very minor comments, one small refactor comment.

ChristopherChudzicki commented 6 years ago

I think this is ready for re-review.

jolyonb commented 6 years ago

LGTM 👍

jolyonb commented 6 years ago

I think these last few changes have made implementing #74 much easier now, too.