Fraction context and FormulaWithUnits

[Alex-Jordan]

Suppose a problem has an answer like 1/2 min. If you use parserNumberWithUnits.pl, it won't keep the fraction form, and that is important in a certain pedagogical context I am working with.

So I tried something like this, combining parserFormulaWithUnits.pl and contextFraction.pl.

DOCUMENT();
loadMacros(qw(PGstandard.pl PGML.pl parserFormulaWithUnits.pl contextFraction.pl));

Context("Fraction");
$answer = FormulaWithUnits("1/2 min");

BEGIN_PGML
Enter [|1/2 min|]*.

[_]{$answer}{16}
END_PGML

ENDDOCUMENT();

And submitting the correct answer, it is not accepted as correct. I think #1107 might make this moot, in that it could offer a better way to have a fraction with units. Also, it looks like I can accomplish what I want with FormulaWithUnits and Numeric context where I set reduceConstants to 0. But this did seem like a possible bug to report.

[drgrice1]

With #1107 and #1108 you can do

DOCUMENT();

loadMacros('PGstandard.pl', 'PGML.pl', 'contextFraction.pl', 'contextUnits.pl', 'PGcourse.pl');

Context(context::Fraction::extending('Units'))->withUnitsFor('time');

$ans = Compute('1/2 min');

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

ENDDOCUMENT();

I think that achieves what you are asking. So I think that #1107 and #1108 do make this moot.

On a side note, I think that MathQuill will need some more work on things related to entering units. I noticed that typing min gets turned into the math min operator. That is actually fine and works. However, it is not okay if you try to type the whole word minutes as that becomes min followed by the text utes, and is submitted with a space between min and utes. Also if you try to type 30 sec the sec becomes the secant function and is submitted with parenthesis which makes the answer correct. Furthermore, if you try to type seconds you get sec(onds).

I think that the min operator can probably just be removed from MathQuill, but I am not sure what to do about the seconds issues.

[dpvc]

I agree that the new units context should resolve the issue. But I took a look into it just to see what was happening, and the issue is with these lines:

pg/lib/Parser/Legacy/NumberWithUnits.pm

Lines 368 to 372 in 10746b1

	my $other_value = $other->value;
	$other->{data}[0] = $other_value * $self->{student_units_ref}{factor} / $self->{units_ref}{factor};
	$ret = $self->SUPER::compare($other, $flag);
	$other->{data}[0] = $other_value;
	return $ret;

With FormulaWithUnits("1/2 min") and a student entering 1/2 min, the $other value will be a Fraction object, and that means $other->value is an array consisting of the numerator and denominator, while this bit of code assumes the value is coming from a Real object, where $other->value is just the perl real for that MathObject. So

my $other_value = $other->value;

sets $other_value to 2 because $other_value is a scalar, so assigning an array to it gives it the length of the array. Then the next line makes the numerator of the $other fraction be equal to 2 times the comparison factor. In this case, that makes the fraction be 2/2, which is not correct.

The modifications of the internals of $other (and $self in the compare() method here and for the NumberWithUnits object earlier) is more complicated than necessary, and can leads to this problem. I understand that it was introduced to avoid the problem that occurred because the original code modified the correct answer value using the factors; that was a bad idea, and needed correcting for sure. But there is a better way to handle this.

As it currently stands, the usual cmp_parse() is replaced by one that removes the units from the student answer and then calls the standard cmp_parse() to check that the numeric (or formula) value of the student answer matches that of the correct answer. The standard cmp_parse() eventually calls the correct answer's compare() method to determine if the student answer is equal to it (after doing some other setup). Originally, the compare() method wasn't overridden, and so the version from the Real or Function object was used, and since those don't know anything about the units, it was just a numeric comparison. The correct answer's number or formula was modified by the proper factor to make the proper numeric comparison for the units that the student had given (and it was that adjustment that the new compare() functions was meant to avoid).

The better approach is to make the compare() function be a proper comparison of numbers or formulas with units, so that the standard cmp_equal call that eventually does $correct == $student to decide if they are equal will return the correct value without having to modify the correct answer's numeric value. This has the side advantage of making it possible to compare numbers or formulas with units directly (via == or even > and <, etc.), which is not currently supported.

The key to making compare() work that way is to use the standard Value::checkOpOrderWithPromote() in order to make sure both values are NumberWithUnits or FormulaWithUnits objects and to get them in the right order. Then check that the base units match, and compare the numeric values multiplied by the needed factors. This doesn't change the value of the original MathObjects, while still getting the proper results.

The only difficulty is that the units do still need to be split off the student number originally, since the parser doesn't handle them, and then the student answer needs to be turned into a NumberWithUnits or FormulaWithUnits object before the comparison is made. The first step is still done in cmp_parse(), and the second is in a new cmp_preprocess() step, which occurs after the student answer has been parsed, but before the comparison is made.

Here is a patch file that implements this idea:

NumberWithUnits.patch

index 3fa90fbe..1eb38f62 100644
--- a/lib/Parser/Legacy/NumberWithUnits.pm
+++ b/lib/Parser/Legacy/NumberWithUnits.pm
@@ -69,8 +69,10 @@ sub new {
        $num = $self->makeValue($num, context => $context);
        my %Units = getUnits($units);
        Value::Error($Units{ERROR}) if ($Units{ERROR});
+       my @baseUnits = grep {/./} (map { $Units{$_} && $_ ne 'factor' ? $_ : '' } keys %Units);
+       $num->{baseUnits} = join(',', map {"$_=$Units{$_}"} main::lex_sort(@baseUnits));
        $num->{units}     = $units;
-       $num->{units_ref} = \%Units;
+       $num->{factor}    = $Units{factor};
        $num->{isValue}   = 1;
        $num->{correct_ans}              .= ' ' . $units           if defined $num->{correct_ans};
        $num->{correct_ans_latex_string} .= ' ' . TeXunits($units) if defined $num->{correct_ans_latex_string};
@@ -186,46 +188,37 @@ sub cmp_parse {
                $self->cmp_Error($ans, "Your units can only contain one division");
                return $ans;
        }
-       my %Units = getUnits($units);
-       if ($Units{ERROR}) { $self->cmp_Error($ans, $Units{ERROR}); return $ans }
-
-       # Check the numeric part of the answer.
-       $ans->{student_ans}        = $num;
-       $self->{student_units_ref} = \%Units;
+       $ans->{units}       = $units;
+       $ans->{student_ans} = $num;
+       $ans                = $self->cmp_reparse($ans);
 
-       $ans = $self->cmp_reparse($ans);
+       $self->cmp_Error($ans, "The units for your answer are not correct")
+               if $ans->{ans_message} eq ''
+               && $ans->score eq 0
+               && !$ans->{isPreview}
+               && $ans->{student_value}{baseUnits} ne $ans->{correct_value}{baseUnits};
 
-       delete $self->{student_units_ref};
+       return $ans;
+}
 
-       # Adjust the answer strings.
+sub cmp_preprocess {
+       my $self  = shift;
+       my $ans   = shift;
+       my $units = $ans->{units};
+       $ans->{student_value} = $self->new($ans->{student_value}, $units);
        $ans->{student_ans}          .= " " . $units;
        $ans->{preview_text_string}  .= " " . $units;
        $ans->{preview_latex_string} .= '\ ' . TeXunits($units);
-
-       return $ans unless $ans->{ans_message} eq '';
-       #
-       #  Check that we have an actual number, and check the units
-       #
-       if (!defined($ans->{student_value}) || $self->checkStudentValue($ans->{student_value})) {
-               $ans->{student_value} = undef;
-               $ans->score(0);
-               $self->cmp_Error($ans, "Your answer doesn't look like a number with units");
-       } else {
-               $ans->{student_value} = $self->new($num, $units);
-               foreach my $funit (keys %{ $self->{units_ref} }) {
-                       next if $funit eq 'factor';
-                       next if $self->{units_ref}{$funit} == $Units{$funit};
-                       $self->cmp_Error($ans, "The units for your answer are not correct")
-                               unless $ans->{isPreview};
-                       $ans->score(0);
-                       last;
-               }
-       }
-       return $ans;
 }
 
 sub cmp_reparse { Value::cmp_parse(@_) }
 
+sub compare {
+       my ($self, $l, $r) = Value::checkOpOrderWithPromote(@_);
+       return $l->{baseUnits} cmp $r->{baseUnits} unless $l->{baseUnits} eq $r->{baseUnits};
+       return $self->demote($l) * $l->{factor} <=> $self->demote($r) * $r->{factor};
+}
+
 sub add_fundamental_unit {
        my $unit = shift;
        $fundamental_units->{$unit} = 0;
@@ -270,6 +263,8 @@ sub makeValue {
        my $value   = shift;
        my %options = (context => $self->context, @_);
        my $num     = Value::makeValue($value, %options);
+       $num = Value::makeValue($num->eval, %options)
+               if $num->type eq 'Number' && !Value::classMatch($num, 'Real', 'Formula');
        Value::Error("A number with units must be a constant, not %s", lc(Value::showClass($num)))
                unless Value::isReal($num);
        return $num;
@@ -278,7 +273,7 @@ sub makeValue {
 sub checkStudentValue {
        my $self    = shift;
        my $student = shift;
-       return $student->class ne 'Real';
+       return $student->isReal;
 }
 
 sub promote {
@@ -290,26 +285,10 @@ sub promote {
        return $self->new($context, $x, @_);
 }
 
-# This saves the current value of the student answer, and then compares the answers with the parent compare method.
-# Then the student value is put back to what it was.  The correct value will be the one that has the units refs.
-sub compare {
-       my ($self, $other, $flag) = @_;
-
-       if (defined $self->{units_ref} && defined $self->{student_units_ref}) {
-               my $other_value = $other->value;
-               $other->{data}[0] = $other_value * $self->{student_units_ref}{factor} / $self->{units_ref}{factor};
-               my $ret = $self->SUPER::compare($other, $flag);
-               $other->{data}[0] = $other_value;
-               return $ret;
-       } elsif (defined $other->{units_ref} && defined $other->{student_units_ref}) {
-               my $self_value = $self->value;
-               $self->{data}[0] = $self_value * $other->{student_units_ref}{factor} / $other->{unit_ref}{factor};
-               my $ret = $self->SUPER::compare($other, $flag);
-               $self->{data}[0] = $self_value;
-               return $ret;
-       }
-
-       return $self->SUPER::compare($other, $flag);
+sub demote {
+       my $self = shift;
+       my $num  = shift;
+       $self->Package("Real")->make($self->context, $num->value);
 }
 
 sub string {
@@ -349,47 +328,10 @@ sub checkStudentValue {
        return $student->type ne 'Number';
 }
 
-# This does much the same as the compare method for a Parser::Legacy::NumberWithUnits object, except that it tries to
-# adjust the formula tree of the student answer.  If the student answer is not a formula, then it adjusts its value.
-# The comparison is made by the parent class method.  Then the adjustment is undone.
-sub compare {
-       my ($self, $other, $flag) = @_;
-
-       if (defined $self->{units_ref} && defined $self->{student_units_ref}) {
-               if (defined $other->{tree}) {
-                       my $other_tree = $other->{tree};
-                       $other->{tree} = $other->Item('BOP')->new($other, '*', $other_tree,
-                               $other->Item('Value')->new($other, $self->{student_units_ref}{factor} / $self->{units_ref}{factor})
-                       );
-                       my $ret = $self->SUPER::compare($other, $flag);
-                       $other->{tree} = $other_tree;
-                       return $ret;
-               } else {
-                       my $other_value = $other->value;
-                       $other->{data}[0] = $other_value * $self->{student_units_ref}{factor} / $self->{units_ref}{factor};
-                       $ret              = $self->SUPER::compare($other, $flag);
-                       $other->{data}[0] = $other_value;
-                       return $ret;
-               }
-       } elsif (defined $other->{units_ref} && defined $other->{student_units_ref}) {
-               if (defined $self->{tree}) {
-                       my $self_tree = $self->{tree};
-                       $self->{tree} = $self->Item('BOP')->new($self, '*', $self_tree,
-                               $self->Item('Value')->new($self, $other->{student_units_ref}{factor} / $other->{units_ref}{factor})
-                       );
-                       my $ret = $self->SUPER::compare($other, $flag);
-                       $self->{tree} = $self_tree;
-                       return $ret;
-               } else {
-                       my $self_value = $self->value;
-                       $self->{data}[0] = $self_value * $other->{student_units_ref}{factor} / $other->{units_ref}{factor};
-                       $ret             = $self->SUPER::compare($other, $flag);
-                       $self->{data}[0] = $self_value;
-                       return $ret;
-               }
-       }
-
-       return $self->SUPER::compare($other, $flag);
+sub demote {
+       my $self = shift;
+       my $num  = shift;
+       $self->Package("Formula")->new($self->context, $num->{tree});
 }
 
 sub string {

[dpvc]

PS, these changes also allow NumberWithUnits("1/2 m") in the Fraction context, but the fraction is converted to a Real internally, since NumberWithUnits is explicitly a subclass of Value::Real, and doesn't inherit any fraction methods. That is the advantage of the new approach to extending contexts provided by contextExtensions.pl and used by contextUnits.pl.