Legacy NumberWithUnits issue in MultiAnswer or RadioMultiAnswer problems.

[drgrice1]

If a NumberWithUnits is a part answer for a MultiAnswer or RadioMultiAnswer that has allowBlankAnswers => 1, then the correct answer for that part is not accepted as correct with the recent changes to NumberWithUnits.pm. I believe the problem is that with the recent changes, the NumberWithUnits uses a pre filter. However, when the allowBlankAnswer => 1 option is used, then all pre filters are removed including the new units pre filter.

The following MWE demonstrates the issue. Of course it doesn't make sense to use a MultiAnswer for a single answer like this, but this is a MWE and the actual problems where this is now an issue have other parts.

DOCUMENT();

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

$x = random(10, 100);

$ansUnit = NumberWithUnits(90, "~~x{00B0}");

$multians = MultiAnswer($ansUnit)->with(
    allowBlankAnswers => 1,
    checker           => sub {
        my ($correct, $student, $ma, $ans) = @_;
        return $correct->[0] == $student->[0];
    }
);

BEGIN_PGML
[`[$ansUnit] = `] [_]{$multians}{10}
END_PGML

ENDDOCUMENT();

With that problem if you enter 90° (use MathQuill and type deg which is changed to the degree symbol) the checker is not called, and instead the message Unexpected character '°' is shown. With this same problem and WeBWorK/PG 2.19 the answer is accepted as correct.

Also, if you remove the allowBlankAnswers => 0 option and enter 90 without the degree unit, then you get the warning

Evaluation error in new process: Answer AnSwEr0001:

UNITS_NONE
::
Your answer doesn't look like a number with units

Note that if you add the checkTypes => 0 option , then you don't even get the message Your answer doesn't look like a number with units at all. Although the warning message is not shown.

Also the checker is called for the answer without the degree unit (a warn statement in the checker reveals this). It was not called in this case for 2.19.

[dpvc]

I suspect you are right that it is the missing pre-filter that is the issue.

Rather than adjusting NumberWithUnits, I'm wondering if the correct solution isn't to adjust MultiAnswer's allowBlankAnswers to look through the pre-filters and only remove the blank one (it is named, so can be identified). I think this line

pg/macros/parsers/parserMultiAnswer.pl

Line 108 in 0c7f42d

$cmp->install_pre_filter('erase');

could be replaced with a loop that looks through @{$cmp->{pre_filters}} for one whose _filter_name is blank_filter. I haven't looked closely to see if there are sub-structures that need to be de-referenced, but I think that might be the better solution. I was always a bit unhappy with erasing all pre-filters, but that was what was possible in the days that MutliAnswer was written (or at least I didn't know enough then to do differently).

[drgrice1]

That is what I was thinking. I will work up a pull request that does that.

[drgrice1]

Actually, I don't think it is as simple as looping through the pre filters and finding the one with the _filter_name of blank_prefilter. The issue is that the entries in the pre_filters array are code references, not the answer hashes, and the _filter_name isn't set until the pre filters are called.

[drgrice1]

I just realized that the last issue I noted with the incorrect answer without units goes further than just with MultiAnswer or RadioMultiAnswer problems. That happens even with the most basic of problems. Consider the following minimal working example:

DOCUMENT();

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

$ans = NumberWithUnits("40 ft");

BEGIN_PGML
Enter [`[$ans]`]: [_]{$ans}.
END_PGML

ENDDOCUMENT();

In that problem if you enter 40, then you get the warning I mentioned before.

[dpvc]

The issue is that the entries in the pre_filters array are code references

That may be why I wasn't able to do it before. But perhaps you can compare the core references to the code reference for the actual definition of the filter function. I'm not sure whether that will be a difference reference or not. Otherwise, the only thing I can suggest is that the blank filter should always be the first one in the list (it is added in the AnswerEvaluator->new() method, so no chance of anything else being before that, unless someone does something very sneaky), so you might just shift the first filter off the list. The only problem I can see is if someone already removed it.

[drgrice1]

Yeah, I have a problem that forces a filter to be the first one regardless of what filters have already been installed. Of course, it just unshifts a filter onto the beginning of the list. Although, it is only a test problem (just messing with things) and none of my actual problems used in production do that.

I will see what I can work out.

[dpvc]

OK, I've looked into the warning message you are getting, and it is due to my use of error_flag. That was intended to stop the processing of filters and evaluators, but it turns out that it also end ups showing the error flag at the end (which I didn't see, as I was using a command-line tester, not an actual running WeBWorK).

Here is a patch that doesn't use error_flag:

diff --git a/lib/Parser/Legacy/NumberWithUnits.pm b/lib/Parser/Legacy/NumberWithUnits.pm
index e67859e8..8df61b97 100644
--- a/lib/Parser/Legacy/NumberWithUnits.pm
+++ b/lib/Parser/Legacy/NumberWithUnits.pm
@@ -92,7 +92,7 @@ sub splitUnits {
                : $aUnit . '(?:\s*[/* ]\s*' . $aUnit . ')*';
        $unitPattern = $unitPattern . '(?:\/' . $unitPattern . ')*' if $parseMathQuill;
        my $unitSpace = "($aUnit) +($aUnit)";
-       my ($num, $units) = $string =~ m!^(.*?(?:[)}\]0-9a-z]|\d\.))\s*($unitPattern)\s*$!;
+       my ($num, $units) = $string =~ m!^(.*?(?:[)}\]0-9a-z]|\d\.))?\s*($unitPattern)\s*$!;
        if ($units) {
                while ($units =~ s/$unitSpace/$1*$2/) { }
                $units =~ s/ //g;
@@ -238,20 +238,26 @@ sub unitsPreFilter {
                $ans->{correct_value}{context}
                        && $ans->{correct_value}->context->flag('useMathQuill')
                        && (!defined $ans->{mathQuillOpts} || $ans->{mathQuillOpts} !~ /^\s*disabled\s*$/i));
+       if (defined($units) && $units ne '' && $num eq '') {
+               $self->cmp_Error($ans, "Units must follow a number");
+               $ans->{unit_error} = $ans->{ans_message};
+               $ans->{student_ans} = '';
+               return $ans;
+       }
        unless (defined($num) && defined($units) && $units ne '') {
                $self->cmp_Error($ans, "Your answer doesn't look like " . lc($self->cmp_class));
-               $ans->{error_flag} = 'UNITS_NONE';
+               $ans->{unit_error} = $ans->{ans_message};
                return $ans;
        }
        if ($units =~ m!/.*/!) {
                $self->cmp_Error($ans, "Your units can only contain one division");
-               $ans->{error_flag} = 'UNITS_DIVISION';
+               $ans->{unit_error} = $ans->{ans_message};
                return $ans;
        }
        my $ref = { getUnits($units) };
        if ($ref->{ERROR}) {
                $self->cmp_Error($ans, $ref->{ERROR});
-               $ans->{error_flag} = 'UNITS_BAD';
+               $ans->{unit_error} = $ans->{ans_message};
                return $ans;
        }
        $ans->{units}       = $units;
@@ -263,6 +269,14 @@ sub cmp_preprocess {
        my $self = shift;
        my $ans  = shift;
 
+       if ($ans->{unit_error}) {
+               $ans->{ans_message} = $ans->{error_message} = $ans->{unit_error};
+               if ($ans->{student_ans} eq '') {
+                       $ans->{student_ans} = $ans->{original_student_ans};
+                       $ans->{preview_latex_string} = TeXunits($ans->{student_ans});
+               }
+               return $ans;
+       }
        my $units = $ans->{units};
        return $ans unless $units;
        $ans->{student_ans}          .= " " . $units;
@@ -272,9 +286,9 @@ sub cmp_preprocess {
        if (!defined($ans->{student_value}) || $self->checkStudentValue($ans->{student_value})) {
                $ans->{student_value} = undef;
                $ans->score(0);
-               $ans->{error_flag} = 'UNITS_NO_NUMBER';
                $self->cmp_Error($ans, "Units must follow a number");
-               return;
+               $ans->{unit_error} = $ans->{ans_message};
+               return $ans;
        }
 
        $ans->{student_formula} = Parser::Legacy::FormulaWithUnits->new($ans->{student_formula}->{tree}, $units);
@@ -284,7 +298,7 @@ sub cmp_preprocess {
 sub cmp_equal {
        my $self = shift;
        my $ans  = shift;
-       if (!$ans->{error_flag}) {
+       if (!$ans->{unit_error}) {
                my $meth = @{ ref($self) . '::ISA' }[-1] . '::cmp_equal';
                $meth = 'Value::cmp_equal' unless defined &$meth;
                &$meth($self, $ans, @_);
@@ -298,7 +312,6 @@ sub cmp_postprocess {
                $self->cmp_Error($ans, "The units for your answer are not correct")
                        unless $ans->{correct_value}->uPowers eq $ans->{student_value}->uPowers;
        }
-       $ans->{error_flag} = undef if $ans->{error_flag} =~ m/^UNITS_/;
        return $ans;
 }

This also allows a unit without a number to be properly identified and a message issued.

Once you deal with the blank filter, I think that will resolve all the issues here.

[dpvc]

It looks like you should be able to use AnswerEvaluator->new()->{pre_filters}[0][0] to get the code reference for the blank filter, so I think you could use

my $blank = AnswerEvaluator->new()->{pre_filters}[0][0];
$cmp->{pre_filters} = (grep {$_->[0] != $blank} @{$cmp->{pre_filters}});

to remove the blank filter.

[drgrice1]

Another way would be to install a post filter to clear the error flag if it is one of the unit errors. Your way is probably better though if the error flag is only for internal use of the NumberWithUnits code.

I will put in a pull request later.

[drgrice1]

Your suggestion for removing the blank check filter will work well.

However, I am seeing some issues with the suggestion to switch the error_flag to an internal value. I have MultiAnswer problems that check the cases that a student enters the correct answer but without the unit, and in that case partial credit is awarded. With webwork/pg 2.19 and before the MultiAnswer checker is still called for answers without the unit if checkTypes => 0 is set. With your suggestion for the error_flag the MultiAnswer checker method is not called even if checkTypes => 0 is set. However, if I leave the error_flag as it is, and do as I suggested and install a post filter that clears the error_flag if it is one of the UNITS_... errors then the checker is called and it works as before.

[drgrice1]

I realized what I said was not quite correct. The MultiAnswer checker is not called by default. But I install a post filter on each answer part that checks to see if the student_value is defined, and if it is not then it attempts to generate a student_value as a regular (non-unit) formula. The difference is that with your suggestion for the error_flag change the student_value is now defined, but previously it was not. If I instead clear the error_flag in a post filter, the student_value is still not defined in the post filter. I think the reason is that if the error_flag is set, then all evaluators (including the MathObject evaluator which calls cmp_parse) are skipped, since the post filter doesn't clear the error_flag until later.

[dpvc]

OK, that's fine, but if you go that route, I'd still recommend adding the "Units must follow a number" check that I added at the top of the patch above. Note that it requires the change to the patter that is just above that. You should also remove the very last line that tries to undo the error_flag but doesn't because the cmp_postfilter never runs in the case there is an error_flag.

[drgrice1]

So I am still investigating which is the better route to go. I don't mind needing to change problems if the change to the units code is the right way to go, and I think that your approach is the right way to go. In looking closer at things I see that with your approach the student answer is parsed into a MathObject even in the case that the student enters a number without a unit. Previously this was not the case, and with my post filter approach that is still not the case. Of course, that is part of what causes an issue in my problems, but that can be fixed in my problems. I realized the issue with your approach in relation to my current problem code is that the ans_message is set, and previously it was not (in addition to the student answer not being parsed into a MathObject). Here is a MWE of the code that I was using:

DOCUMENT();

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

$x = random(10, 100);

$ansUnit = NumberWithUnits($x, "~~x{00B0}");

$multians = MultiAnswer($ansUnit)->with(
    checkTypes        => 0,
    allowBlankAnswers => 1,
    checker           => sub {
        my ($correct, $student, $ma, $ans) = @_;
        return 1
            if Value::classMatch($student->[0], $correct->[0]->class)
            && $correct->[0] == $student->[0];
        if (defined $student->[0]
            && $correct->[0]->value == $student->[0]->value)
        {
            $ma->setMessage(1, 'Your answer is missing the degree symbol.');
            return 0.75;
        }
        return 0;
    }
);

for my $answer (@{ $multians->{cmp} }) {
    $answer = $answer->withPostFilter(sub {
        my $ansHash = shift;

        if (!defined $ansHash->{student_value}) {
            $ansHash->{student_value} =
                eval { Formula($ansHash->{original_student_ans}) };
            $ansHash->{student_ans} = $ansHash->{student_value}
                // $ansHash->{original_student_ans};
            $ansHash->{preview_latex_string} =
                ref($ansHash->{student_ans})
                ? $ansHash->{student_ans}->TeX
                : "\text{$ansHash->{student_ans}}";
            $ansHash->{ans_message} = $ansHash->{error_message} = '';
        }

        return $ansHash;
    });
}

BEGIN_PGML
[`[$ansUnit] = `] [_]{$multians}{10}
END_PGML

ENDDOCUMENT();

That worked with 2.19 and also works with the post filter approach to clearing the error_flag, but doesn't work with your approach. However, if I change the post filters in the problem to

for my $answer (@{ $multians->{cmp} }) {
    $answer = $answer->withPostFilter(sub {
        my $ansHash = shift;
        delete $ansHash->{ans_message}
            if defined $ansHash->{ans_message}
            && $ansHash->{ans_message} eq
            "Your answer doesn't look like a number with units";
        return $ansHash;
    });
}

it fixes things with your approach.

I was thinking that perhaps the Your answer doesn't look like a number with units warning could only be given if the showTypeWarnings context flag is 1. In a sense that warning is a type warning. It is given when the student answer is a Number and a NumberWithUnits is expected. Although, it would also be nice if the MultiAnswer checkTypes option could be linked to the showTypeWarnings context flag in some way. There is also the typeError answer hash flag which is related and perhaps should also be set in this case.

In any case, I think for now, I will go with your approach and make the necessary changes to my problems. Although there is one more thing that doesn't seem right in your suggestion code. That is the code related to the Units must follow a number warning. In your suggestion you add the part in the pre filter for handling that, yet there is still the code in the cmp_preprocess method that also gives that warning. Furthermore, it sets the unit_error answer hash flag after the handling of unit_error flag earlier in that method. It seems pointless to set the unit_error flag at that point since it is handled earlier and never considered again. In that case the ans_message and error_message will be set by the cmp_Error call, and that is after those are reset in the cmp_parse call (unlike those set in the pre filter), and so those will persist in any case.

[drgrice1]

I guess there is one case where the unit_error flag is considered after the cmp_preprocess method call. That is in the cmp_equal method. So I guess it needs to be set for that.