Matrix partial credit

[ChristopherChudzicki]

Adds MatrixEntryComparer, a configurable grader that implements partial credit for matrices based on entry.

[ChristopherChudzicki]

Oops, I need one more test now that MatrixGrader.default_comparer is back to its original.

[codecov-io]

Codecov Report

Merging #255 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #255   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines        2387   2428   +41     
=====================================
+ Hits         2387   2428   +41

[jolyonb]

I should add that I do like the idea of having partial credit for a MatrixGrader configurable through that MatrixGrader. It makes setting a default really easy!

Perhaps the default grader for MatrixGrader should just be MatrixEntryComparer, using settings that would make it essentially equivalent to EqualityComparer. That way, validation can go through as per normal, as the partial credit options don't affect validation at all.

[ChristopherChudzicki]

Thoughts re modifying the default comparer on an instance level

The issue is really that during when voluptuous runs its validator functions on config, the keys can't communicate. So it's hard for config keys entry_partial_<whatever> to influence config['answers'] validation.

In other instances where config key-values need "correlated" validation, we've performed additional validation steps after super().__init__ runs. For example, validate_no_collisions(self.config, keys=['variables', 'user_constants']).

I don't think that's suitable in this case for two reasons:

1. schema_answers will have already set the comparer functions. It bugs me to set the comparer to one thing, then to reset it again later to another thing.
2. More seriously, schema_answers is used by inherited __call__ to coerce except values into answers when provided by the expect XML attribute. So schema_answers needs to be able to construct the correct comparer on its own.
   • (I guess FormulaGrader could shadow call to make its own modifications to the expect value read from XML, but I'd rather not).

Modifying default_comparer on the instance level before super().__init__ runs its voluptuous validation solves both of these problems.

Moreover, I think it conceptually makes sense to modify the default comparer. Consider that in the situation below

grader = MatrixGrader(
    answers = (
        'expect_value_a',
        {'comparer': comparer1, 'comparer_params': ["whatever params"]}
        'expect_value_b',
        {'comparer': comparer2, 'comparer_params': ["whatever params"]}
    ),
    entry_partial_credit='proportional'
)

the comparer MatrixEntryComparer(entry_partial_credit=0.5) should be used for expect_value_a and expect_value_b`, but not for the other two answers, and in this sense it is the default comparer for this instance of MatrixGrader.

[jolyonb]

This sounds reasonable to me. I am wary however of the interaction of your proposal with set_default_grader. Do you have an idea of how you want to reconcile the two?

[ChristopherChudzicki]

Well, set_default_comparer is a classmethod that sets the default for the class, not for the instance. So the only conflict I see is that a user might set a class-level default comparer of NEW_CLASS_DEFAULT and use the entry_partial keys, then be surprised when MatrixEntryComparer was used instead of NEW_CLASS_DEFAULT.

An alternative syntax I considered was to allow FormulaGrader and its subclasses to specify a top-level comparer in the configuration, that would be the instance-level default. So something like:

grader = MatrixGrader(
    answer='A*B',
    variables=['A', 'B']
    comparer=MatrixEntryComparer(partial_credit=0.5)
)

where config['comparer'] become the instance-level default.

The advantage to this approach would be (1) more general: good for FormulaGrader, too. (2) makes more clear that you're overriding the default comparer. But it also requires more direct interaction with comparers, which is a bit advanced, I suppose. I personally like this better.

(To be clear, I'd still be setting default_comparer on the instance level.)

[jolyonb]

Hmm. If anybody is wanting to play with comparers at any stage, they need to get their hands dirty. I think the comparer flag is cleaner than the set_default_comparer method we're currently using (and bonus, can be easily set as a course-wide default!). Up to you if you want to do this. (edit: a nice thing here is that the instance-level default_comparer would never need to be overridden)

At the same time, being able to set the MatrixGrader comparer through simple options is also a really nice touch, because it makes it much more accessible. We can detect if those options were provided before overriding a comparer before validation (what you were proposing, I gather).

Ok, I consider this hashed out. I trust you :-)

[ChristopherChudzicki]

@jolyonb This is ready for re-review. Sorry for force push, I rebased but probably didn't need to.

I'm about to get on a flight to Seattle. If you can review in the next 5 or 6 hours, I'll try to address any comments on my next flight.

[jolyonb]

Hey, no problem. I force push all the time, and enough stuff has been modified the past few days!

Have a good flight! There'll be a thorough review ready for you when you land.

[ChristopherChudzicki]

Thanks for the comments and feedback! I believe I've implemented everything as you suggested.

The vector/matrix feedback messages look good. Actually, tensor looks OK, too, but Asciimath doesn't render it well.

About to take off, so won't be online for another few hours. (Our flight got delayed, so I did this in the layover...)

[codecov-io]

Codecov Report

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

@@           Coverage Diff            @@
##           master   #255      +/-   ##
========================================
+ Coverage   98.52%   100%   +1.47%     
========================================
  Files          29     29              
  Lines        2508   2552      +44     
========================================
+ Hits         2471   2552      +81     
+ Misses         37      0      -37

[codecov-io]

Codecov Report

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

@@           Coverage Diff            @@
##           master   #255      +/-   ##
========================================
+ Coverage   98.52%   100%   +1.47%     
========================================
  Files          29     29              
  Lines        2508   2552      +44     
========================================
+ Hits         2471   2552      +81     
+ Misses         37      0      -37

[jolyonb]

Looks pretty good to me :) Just going through my comments step by step, but will likely merge soon.