List inside a RadioMultiAnswer not working if the list has only one thing.

[Alex-Jordan]

I think this is an issue with parserRadioMultiAnswer.pl. But maybe with Lists.

Consider the exercise below. Imagine a problem where you ask if the MVT applies to a function on a certain closed interval or not. The first option is something like "The MVT does not apply.". The second option is for when it does apply, and there is a list of numbers c where f'(c) = (f(b) - f(a))/(b - a).

That answer blank should expect a List. And for the problem in question, it turns out the list only has one element. Then the problem is not accepting the correct answer as correct.

DOCUMENT();

loadMacros(qw(PGstandard.pl PGML.pl parserRadioMultiAnswer.pl));

Context("Numeric");

$list = List(Real(1));
$list2 = List(Real(1),Real(2));

$rma = RadioMultiAnswer([
    ['No numbers.'],
    ["The list \($list\):$BR %s", $list]
], 1);

BEGIN_PGML
[_]{$rma}
END_PGML
ENDDOCUMENT();

If there are two numbers in the list, then it's OK.

[drgrice1]

This is an issue with both the parserRadioMultiAnswer.pl and parserMultiAnswer.pl macros. The default graders don't work for lists. So you have to provide your own grader in this situation at this point. Eventually, it would be nice to fix this of course.

[drgrice1]

You claim that for a list with two numbers this works, but that isn't actually true. With your example, if you switch to $list2, and then enter 2, 1, that answer is not accepted as correct and it should be with the usual list defaults (ordered is false). Part of the issue in that case is that ordered is an answer checker option, and not a property of the list itself. So if $l1 = List(1, 2) and $l2 = List(2, 1), then $l1 == $l2 is false.

The issue for a single element list is different though. The issue in that case is that the correct answer is a MathObject list since you pass it as such. However, the student answer is not recognized as a list since it does not have the list operator (a comma).

[somiaj]

This is more general than just the multianswer macros, fixing it so unordered lists worked with the overloaded == operator would be nice, as this also causes other problems.

[dpvc]

Yes, the == comparison between lists is the source of the issues, here. One of the reasons the unordered lists are not handled by that is because the == operator only generates errors when the comparison is impossible, while the answer checker version produces hints about which entries are correct and which aren't, and whether there are the right number of entries, or the right delimiters, and so on. It would have meant duplicate code to handle the two cases separately. (One possibility for handling unordered lists via == would be to first sort the lists and then compare.)

Originally, the MultiAnswer object required you to provide a checker function, so it was up to the author to properly handle lists. One reason that there was not a default checker was that the intent for MultiAnswer was that there was expected to be some interaction among the answers, and that only the author would know what that was. If the only criterion was that the student answers matched the correct ones, then that could have been handle without the MultiAnswer as there is no connection that needs to be checked. If the goal was to have the answers be marked correct only when all are correct, a more simple And object could have been made to do that. The only other reason I could see for it was in the formatting of the parts of an answer in the answer table, but that is now gone, so even that is no longer much of a reason to use MultiAnswer. I suppose the singleResult option is also obsolete at this point, now that the results table is no more.

So with that in mind, I think the current default checker function is the heart of the problem with its use of == for checking the answers. In particular, for lists, the handling of unordered lists and the promotion of a single number to a singleton list are both performed in the answer checker code, not the == code. So rather than using == it would be better to use each of the correct answer's cmp->evaluate() methods to determine if the student answer matches. That would use the answer-checker processing, with all its useful error messages, rather than the == processing.

Fortunately the MultiAnswer object has already called cmp->evaluate() for each answer in order to detect syntax or other errors, and it retains the resulting answer hashes in the $self->{ans} array. So the default checker can call on those values rather than performing an equality check again.

There is a problem, however, which is that the scores are zeroed out in the $self->{ans} array, and I no longer remember why that might have been important (that code is about 20 years old). I don't think zeroing it is actually important as the score is never used, but I may have overlooked something.

There is also a second issue which is the checker => sub {0} in the @ans_defaults array. This was probably intended to prevent the actual check from being performed (for efficiency) so that the cmp->evaluate() calls were only performing syntax and type checks, but that prevents the resulting answer hashes from having the correct scores (and it prevents a list check from every being marked correct during that evaluation, since the tests for each entry are always scored as 0, so no correct entries are found).

So three small changes could be used to correct the issues raised here, at least for MultiAnswer:

diff --git a/macros/parsers/parserMultiAnswer.pl b/macros/parsers/parserMultiAnswer.pl
index 3472f64a..68a46164 100644
--- a/macros/parsers/parserMultiAnswer.pl
+++ b/macros/parsers/parserMultiAnswer.pl
@@ -14,7 +14,6 @@ our $answerPrefix = "MuLtIaNsWeR_";    # answer rule prefix
 our $separator    = ';';               # separator for singleResult previews
 
 my @ans_defaults = (
-       checker             => sub {0},
        showCoordinateHints => 0,
        showEndpointHints   => 0,
        showEndTypeHints    => 0,
@@ -93,7 +92,7 @@ sub cmp {
                        my @scores;
 
                        for (0 .. $self->length - 1) {
-                               push(@scores, $correct->[$_] == $student->[$_] ? 1 : 0);
+                               push(@scores, $self->{ans}[$_]{score});
                        }
                        return \@scores if $self->{partialCredit};
                        for (@scores) {
@@ -280,7 +279,6 @@ sub entry_check {
        my $ANS = $self->{cmp}[$i]->evaluate($ans->{student_ans});
        $self->{ans}[$i] = $ANS;
        $ANS->{type} = $ans->{type};
-       $ANS->score(0);
 
        foreach my $id (keys %{$ans}) {
                $ANS->{$id} = $ans->{$id} unless defined($ANS->{$id});

I have not looked closely at parserRadioMultiAnswer.pl, but I assume something similar needs to be done there.

[drgrice1]

The singleResult option is certainly not obsolete. There are many cases where the answers are interlinked in such a way that you can't say that one particular answer is right or wrong with consideration of the the others, and messages provided in the checker don't pertain to a single answer, but to the group of answers. In that case it is not desirable for each answer to have its own feedback button. It is better for the group to have a single feedback button with any messages that might be provided by the checker. Note that with the parserRadioMultiAnswer.pl macro is no singleResult option, but it is always single result in the sense of the option for the parserMultiAnswer.pl macro as the answers are always related in such a way (bound by the overlying radio button answer) that the separation can't be made.

I generally still believe that the primary usage of MultiAnswer is when the parts are related in some way that only the author knows, and that a checker should be provided in most cases. While that is true to an extent with the parserRadioMultiAnswer.pl macro, there are many cases where the only real answers are the radio button answer and then one primary part answer (that is only applicable if the correct radio button is chosen). So a default checker can handle those situations.

[drgrice1]

Also, formatting multiple answers into a single feedback can be useful by itself. That is a scenario where the singleResult is useful with parserMultiAnswer.pl. In fact, the screenshot below is of a problem that I use the parserMultiAnswer.pl macro for just that.

[somiaj]

My primary use of parserMultiAnswer is for the singleResult => 1 option, to link multiple answers into a single one to both provide feedback and format the answer preview/output as a single answer with multiple answer blanks. Although lots of my problems provide a custom checker, I found that for some problems I was using just the default checker and using parserMultiAnswer to link the problems into a single answer/output. One problem type I used a lot of were fill in the blank answers using drop down menus to create a correct statement, where a full statement was either correct or incorrect, and thought having a default checker would help simplify that use case.

I still think it would be nice to allow == to do unordered list comparisons, even if it doesn't include a partial credit option that a full list_checker would. My understanding is this is partly why using list of lists (I wanted to use them to have sets of sets or sets of ordered pairs) would require a lot of custom checkers.

I have thought about ordering the lists before comparing them option, which will work in lots of cases. But what if the items were formulas like 2x+3y,3x+2y, the way the student entered the formula could affect the order, such as if they entered 3y+2x,2y+3x.

[dpvc]

@drgrice1: OK, I stand corrected about singleResult. I did say that formatting the answers was one reason for that, and I see that you have done a lot with that option. I always feel rather good when I see that people are able to take things much farther than I originally envisioned.

@somiaj: You are certainly correct about the formula example. It is possible to use $f->cmp->evaluate($g->string)->{score} to perform the unordered check that you want for $f == $g. Not ideal, but possible to do today.

[drgrice1]

@dpvc: Something is not working right with the code you suggested. Consider the following example:

DOCUMENT();

loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl');

$list  = List(Real(1));
$list2 = List(Real(1), Real(2));

$ma = MultiAnswer($list, $list2)->with(
    checker => sub {
        my ($correct, $student, $ans, $self) = @_;
        warn 'my checker';
        return 0;
    }
);

BEGIN_PGML
Enter the list [`[$list]`]: [_]{$ma}

Enter the list [`[$list2]`]:[_]{$ma}
END_PGML

ENDDOCUMENT();

The checker defined in the problem is never executed, as the warn statement in the checker never happens. However, if the correct answers are entered, they are marked correct.

If you add checkTypes => 0 in the with call, then the checker is executed, but only in certain cases, such as the case that all of the correct answers are entered (and then the checker returns 0, so all are marked incorrect!). If only one correct answer is entered, but the others are left blank, the the checker is not called, but the correct answer is marked correct. That shouldn't happen if a checker is provided.

[drgrice1]

@Alex-Jordan: The example you posted actually does work with the default grader for the singleton list if you add ->with(checkTypes => 0) to the $rma object.

[dpvc]

Well, I thought I had tested that case, but apprently didn't do a very good job of it. I admit to being a bit rushed at the time, so wasn't as thorough as I should have been.

The issue, here, is in the handling of checkTypes. When checkTypes => 1, the user's checker is called only if all the types match exactly, so when List(Real(1)) is matched against the user's Real(1) answer, the types don't agree, the checker is never performed, and the answers are considered to be wrong, even though the student answer could be promoted to a singleton list. (The same would happen if a vector answer received a point instead, for example, even though the point could be promoted to a vector).

The idea for checkTypes was so that the user's checker was guarnateed to know the type of the answer, and could call its methods or otherwise work with it without having to worry about what type the student's answer actually is. But it does have these side effects, I'm afraid.

It would be nice to allow student answers that are compatible with the correct answer. That can be checked using $correct->typeMatch($student), which is true of the $student answer can be compared to the $correct answer in a meaningful way. For example List(1)->typeMatch(Real(1) is true, since the real can be promoted to a singleton list.

So I propose allowing checkTypes => 'compatible' to make that check, while checkTypes => 1 and checkTypes => 'equal' would do the current typechecking, and checkTypes => 0 would do no typechecking. The default still probably needs to be equal type-checking, since that is the current behavior, and changing that could break things too readily. But allowing checkTypes => 'compatible' would allow these list situations to work more easily.

As for the issue with blank entries, that was due to my removing the $ANS->score(0) call, which I thought was not needed, but I neglected to take into account the case where the type-checking failed and the answer hashes were left with their original scores (leading to correct ones being marked correct). So that needs to be handled better. One way is to record the scores before zeroing them out, so that they can be used in the checker, if desired. That actually simplifies the default checker.

Here is a more extensive set of changes that I think will address the issues. In this case, the zeroing of the answer scores is done in the perform_check() method in a new loop (so all of them get zeroed before any type-checking occurs).

diff --git a/macros/parsers/parserMultiAnswer.pl b/macros/parsers/parserMultiAnswer.pl
index 3472f64a..df3ef601 100644
--- a/macros/parsers/parserMultiAnswer.pl
+++ b/macros/parsers/parserMultiAnswer.pl
@@ -14,7 +14,6 @@ our $answerPrefix = "MuLtIaNsWeR_";    # answer rule prefix
 our $separator    = ';';               # separator for singleResult previews
 
 my @ans_defaults = (
-       checker             => sub {0},
        showCoordinateHints => 0,
        showEndpointHints   => 0,
        showEndTypeHints    => 0,
@@ -90,13 +89,8 @@ sub cmp {
                die "Your checker must be a subroutine." if defined($self->{checker});
                $self->{checker} = sub {
                        my ($correct, $student, $self, $ans) = @_;
-                       my @scores;
-
-                       for (0 .. $self->length - 1) {
-                               push(@scores, $correct->[$_] == $student->[$_] ? 1 : 0);
-                       }
-                       return \@scores if $self->{partialCredit};
-                       for (@scores) {
+                       return $self->{scores} if $self->{partialCredit};
+                       for (@{$self->{scores}}) {
                                return 0 unless $_;
                        }
                        return 1;
@@ -280,7 +274,6 @@ sub entry_check {
        my $ANS = $self->{cmp}[$i]->evaluate($ans->{student_ans});
        $self->{ans}[$i] = $ANS;
        $ANS->{type} = $ans->{type};
-       $ANS->score(0);
 
        foreach my $id (keys %{$ans}) {
                $ANS->{$id} = $ans->{$id} unless defined($ANS->{$id});
@@ -305,21 +298,48 @@ sub perform_check {
        my $rh_ans  = shift;
        my $context = $self->context;
        $context->clearError;
+
+       #
+       # Get correct and student answers, and scores.
+       # Initially mark all answer blanks as wrong.
+       #
        my @correct;
        my @student;
-       foreach my $ans (@{ $self->{ans} }) {
+       $self->{scores} = [];
+       for my $ans (@{ $self->{ans} }) {
                push(@correct, $ans->{correct_value});
                push(@student, $ans->{student_value});
+               push @{$self->{scores}}, $ans->{score};
+               $ans->score(0);
+       }
+
+       #
+       # Check for error messages and correct answer types
+       #
+       my $checkTypes = $self->{checkTypes};
+       $checkTypes = 'equal' if $checkTypes && $checkTypes ne 'compatible';
+       for my $ans (@{ $self->{ans} }) {
                return if $ans->{ans_message} || !defined($ans->{student_value});
                return
-                       if $self->{checkTypes}
+                       if $checkTypes eq 'equal'
                        && $ans->{student_value}->type ne $ans->{correct_value}->type
                        && !($self->{allowBlankAnswers} && $ans->{student_ans} !~ m/\S/);
+               return
+                       if $checkTypes eq 'compatible'
+                       && !$ans->{correct_value}->typeMatch($ans->{student_value}, $ans)
+                       && !($self->{allowBlankAnswers} && $ans->{student_ans} !~ m/\S/);
        }
+
+       #
+       # Set the isPreview flag.
+       #
        my $inputs = $main::inputs_ref;
        $rh_ans->{isPreview} = $inputs->{previewAnswers}
-               || ($inputs_{action} && $inputs->{action} =~ m/^Preview/);
+               || ($inputs->{action} && $inputs->{action} =~ m/^Preview/);
 
+       #
+       # Call the user's answer checker.
+       #
        Parser::Context->current(undef, $context);                                    # change to multi-answer's context
        my $flags = Value::contextSet($context, $self->cmp_contextFlags($rh_ans));    # save old context flags
        $context->{answerHash} = $rh_ans;                                             # attach the answerHash
@@ -328,16 +348,20 @@ sub perform_check {
        $context->{answerHash} = undef;                                               # remove answerHash
        if (!@result && $context->{error}{flag}) { $self->cmp_error($self->{ans}[0]); return 1 }
 
+       #
+       # Mark the answers as correct or incorrect.
+       #
        my $result = (scalar(@result) > 1 ? [@result] : $result[0] || 0);
        if (ref($result) eq 'ARRAY') {
                die "Checker subroutine returned the wrong number of results"
                        if (scalar(@{$result}) != $self->length);
-               foreach my $i (0 .. $self->length - 1) { $self->{ans}[$i]->score($result->[$i]) }
+               for my $i (0 .. $self->length - 1) { $self->{ans}[$i]->score($result->[$i]) }
        } elsif (Value::matchNumber($result)) {
-               foreach my $ans (@{ $self->{ans} }) { $ans->score($result) }
+               for my $ans (@{ $self->{ans} }) { $ans->score($result) }
        } else {
                die "Checker subroutine should return a number or array of numbers ($result)";
        }
+
        return;
 }

[drgrice1]

@dpvc: I will probably apply your patch, and put in a pull request. In addition I will make corresponding changes to the parserRadioMultiAnswer.pl patch. However, I want to make one change to your code. I plan to add $self->{checkTypes} = 'compatible' if $self->{checkTypes}; in the case that the default checker is used. That just seems convenient for those using the default checker and for which the "compatible" setting is appropriate.

[dpvc]

@drgrice1, sounds good.

[drgrice1]

There is a case that is still not quite working right with the suggested code. With the problem

DOCUMENT();

loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl');

$list  = List(1);
$list2 = List(1, 2);

$ma = MultiAnswer($list, $list2);
#$ma->setCmpFlags(2, showLengthHints => 0);

BEGIN_PGML
Enter the list [`[$list]`]: [_]{$ma}

Enter the list [`[$list2]`]:[_]{$ma}
END_PGML

ENDDOCUMENT();

if you enter 1 for the first answer and 3 for the second answer, then the first answer is counted correct and the second is incorrect. However, if you enter 1 for the first answer and 1 for the second answer, then both are counted incorrect. This is because $showPartialCorrectAnswers is true and the second answer is awarded 50% credit by the default list answer evaluator, but that also comes with the message "There should be more numbers in your list". Since ans_message is set, the perform_check method returns, and the checker is still not called. Would it be appropriate to not bail on a message if the answer has a nonzero score? You have to track position in the type checking loop to access the score saved in $self->{scores}, but that could be done. Or do you have a better idea?

Note that you can uncomment the commented out line in the above code, and then the answer is given partial credit. However, then you lose the message that it seems still should be given.

[dpvc]

I actually wondered about that situation myself, as I was concerned that the loop

                        for (@{$self->{scores}}) {
                                return 0 unless $_;
                        }
                        return 1;

in the default checker would return 1 as long as there was at least partial credit for the answers. It might be better to use

                        for (@{$self->{scores}}) {
                                return 0 unless $_ == 1;
                        }
                        return 1;

so that partial credit does not count as correct.