Add typecheck to union answer checking

[dpvc]

This PR adds a type check to the Union answer checking so that if the student's answer is not of the correct type, the usual type-mismatch message will be produced. This resolves an issue raised in the WeBWorK forum where a union would be marked correct when it was entered as a list with no braces. This was due to the fact that the Union checker overrides the cmp_equal method that does the usual type checking, and calls the List answer checker, which uses cmp_compare to determine when individual sets in the union are matched. The cmp_compare function does not do the type checking, and its comparison allows more type coercion than cmp_equal does, so that was allowing {1,2,3} to match 1,2,3 in the union.

This PR fixes that by adding a type check in the cmp_equal method for the Union.

You can check it by using the example in the link above, or by using

Context("Interval");
$U = Union(Set(1,2), Set(3,4));
$ans = $U->cmp->evaluate("1,2,3,4");
TEXT($ans->{score}, " ", $ans->{ans_message});

Without the patch, the score will be 1 and there is no answer message. With the patch, the score will be 0, and there should be an error message about the answer being a list of numbers rather than a set, interval, or union.

[taniwallach]

Tested with both the test code provided by @dpvc in the conversation thread, and with the original sample file posted in the forum post. PG files for both are in the
zip file test_492.zip.

The patch works are claimed.

[drgrice1]

This looks good. I tested with both of the files in taniwallach's zip, and it works as claimed.

[drgrice1]

I just noticed that this pull request partially breaks a custom grader that I have been using. Previously if the correct answer is a union, then the list_checker routine was called even if the student's answer was a list of intervals. I used that to give partial credit in this case. For example, if the answer is (-inf,3)U(5,inf) and the student gives (-inf,3),(5,inf). With this pull request that list answer no longer makes it into the list_checker subroutine.

I realize that it was probably unintentional that the list answer was passed to the checker routine before, but is there a simple way to get the old behaviour back for these problems, or will I need to rewrite these problems so that grading is done in a post filter.

[dpvc]

@drgrice1, can you provide an example of the custom grader you are using? There are a couple of approaches that could be taken, but which to use may depend on what you are currently doing.

[drgrice1]

Here is a minimal example of what I was doing:

DOCUMENT();
loadMacros("PGstandard.pl", "MathObjects.pl", "PGML.pl");
TEXT(beginproblem());

$a = 2;
$b = 6;

Context('Interval');

$answer = Compute("(-inf, $a)U($b, inf)");

$answer_cmp = $answer->cmp(
    list_checker => sub
    {
        my ($correct, $student, $ans, $value) = @_;

        return 0 if ($ans->{isPreview} || !(Value::classMatch($ans->{student_value}, 'Interval') ||
                Value::classMatch($ans->{student_value}, 'Union') ||
                Value::classMatch($ans->{student_value}, 'List')));

        my $score = 0;

        # Previously the grader would accept lists of intervals as well as lists of numbers in
        # addtion to single intervals or unions of intervals.
        # So answers like "(-inf,2),(6,inf)" and "2,6" would get here and I would give partial credit and
        # add appropriate messages to those answer.
        # Now only intervals or unions of intervals make it to this point.

        return 2;
    },
    partialCredit => 1, showUnionReduceWarnings => 0
);

BEGIN_PGML
Solve the following compound inequality.  Express the answer in interval notation.

>>[`x < [$a]`] or [`x > [$b]`]<<

The solution set is [_]{answer=>$answer_cmp, width => 10}.
END_PGML

ENDDOCUMENT();

[drgrice1]

I should add that this is not something that I am attempting to do with new problems. This is something that I have been doing with many problems. The point is to make these graders work again with minimal effort.

Is there a way to override the Value::Union package and set the cmp_equal method back to the way it was for these problems?

[drgrice1]

I found a quick fix that works. I added the following to the problem:

package my::Value::Union;
our @ISA = qw(Value::Union);

sub cmp_equal {
	my $self = shift; my $ans = shift;
	my $error = $self->cmp_checkUnionReduce($ans->{student_value}, $ans);
	if ($error) { $self->cmp_Error($ans, $error); return; }
	Value::List::cmp_equal($self,$ans);
}

package main;

Context()->{value}{Union} = "my::Value::Union";

This seems to retrieve the old behavior in these problems for now.

[dpvc]

I found a quick fix that works

Yes, that was what I was going to suggest. Kudos for working it out on your own!

You could put this into a macro file (say templates/macros/originalUnion.pl) that contains

package my::Value::Union;
our @ISA = qw(Value::Union);

sub cmp_equal {
	my $self = shift; my $ans = shift;
	my $error = $self->cmp_checkUnionReduce($ans->{student_value}, $ans);
	if ($error) { $self->cmp_Error($ans, $error); return; }
	Value::List::cmp_equal($self,$ans);
}

package main;

$context{Interval} = Parser::Context->getCopy("Interval");
$context{Interval}->{value}{Union} = "my::Value::Union";

Then you can just use loadMacros("originalUnion.pl"); in the files that need them and the change will be automatic. It would also be possible to put this into parserCustomization.pl and have it ALWAYS be done, but I suspect that that is overkill.

[dpvc]

PS, sorry I didn't get back to you last night; although I knew what needed to be done, I was not able to get to it until today.

[drgrice1]

Yeah, no problem on not getting back to me. I knew I had seen the way to do this somewhere, but hadn't found it yet when I asked. I did eventually find what I was looking for.

Also, I wasn't entirely honest. I did put the code into a macro instead of putting that into every problem that uses this. Those problems already use a custom macro related to this anyway.

[dpvc]

Sounds like you are well on top of it, then. I can retire!

[drgrice1]

I hope not yet. I have learned quite a bit about the parser and MathObjects stuff, but I still have a lot to learn. As the original author you still are the primary expert.