Another legacy NumberWithUnits issue.

[drgrice1]

This was observed for the problem Library/Michigan/Chap2Sec1/Q17.pg. That problem uses a custom checker for the second answer in the problem. The checker tries to convert the student unit answer to a MathObject Real by calling Real($student). I think that used to result in a MathObject Real at least, but now it is still a NumberWithUnits. The checker in the problem is:

ANS(
    NumberWithUnits($v, "ft/s")->cmp(
        checker => sub {
            my ($correct, $student, $ansHash) = @_;
            $stu_real = Real($student);
            if ($min <= $stu_real && $stu_real <= $max) {
                return 1;
            } else {
                return 0;
            }
        }
    )
);

In any case, the correct answer is not accepted anymore. It worked for previous versions of PG. I have changed the call to $stu_readl = $student->value on my server which works, but this is a problem that most likely is used elsewhere.

[drgrice1]

Note that this is unaffected by the changes in #1302. It does not work with main/develop or that pull request.

[dpvc]

Actually, this problem only accepted the correct answer if the student used units of ft/s. If they gave their answer in, say, inches per second, the correct answer would be marked wrong.

Before the recent changes, $student would already have been a Real MathObject, so Real($student) was redundant, and it didn't really make a difference. But after the changes, $student is an actual NumberWithUnits object, not a Real, but because it inherits from Real, the Value::isReal() function is true for it (that is MathObejcts considers it to be a Real). Such behavior is needed for things like Fraction objects to work properly (so everyone who checks for a real operands, for example, doesn't have to also check for fractions explicitly). This is true for numbers with units as well so that most things that accept reals (like additions and multiplication, comparisons, and function calls) will accept numbers with units (without this, the numbers with units would have to override many checks at a higher level than it does now).

The Real() function checks if its argument is already a real and returns that if so. That means it returns the NumberWithUnits that it was passed. A context can override the Real constructor and could use that to check for a NumberWithUnits and demote it to a Real. The problem is that the NumberWithUnits macro file doesn't use a context of its own, it applies to whatever context is in operation, so that makes things tricker.

There are several possible solutions to choose from:

1. The Real object's new() method could check for NumberWithUnits objects specifically, and demote them. Although this is an easy fix, I don't like this solution, as it means the Real object has to be aware of NumberWithUnits, and that breaks the independence from other classes that it currently enjoys.
2. The NumberWithUnits object could modify the context in which it is created in order to override the Real constructor with one that can check for NumberWithUnits objects. This is a hack, and it is always tricky to modify an unknown context, but it could probably be made to work, and leaves the Real object definition untainted.
3. The Real object could provide a hook that other classes can use to handle demotion to a real, and it would call that if the object it is passes has an implementation for that hook. For example, it could look to see if the object has a toReal() method and call that if so, otherwise do what it currently does. This is probably the best solution, as it could be used by other objects, like fractions, complex numbers, etc., though that would have to be checked carefully, as some objects may rely on the current behavior in unexpected ways. We would want it to occur only when an explicit Real() method is called, as new() is used in many other situations where demotion might not be wanted.

If option 3 is taken, I'd recommend adding a new method (e.g., constructor()) that is called by the constructor functions like Real(). They currently do something like Value->Package("Real()")->new(@_) but instead would do Value->Package("Real()")->constructor(@_). Then the base Value object would get a new constructor() method that checks if it has one arguments that is a MathObject, and if so, looks to see if that has a method "toName" where Name is $self->class. If so, it calls that method with the given argument and returns the result, otherwise it calls $self->new(@_).

Then NumberWithUnits can have a toReal() method to demote it to that.

Note that Value->Package("Real()") returns Value->Package("Real") if Real() is not explicitly defined in the context, but Real is. So this should be backward compatible. But it is a more extensive change than simply trying to fix NumberWithUnits.

[drgrice1]

Is this really a valid issue then? Perhaps this problem was just coded incorrectly, and only worked right mostly by chance. Should the NumberWithUnits code be changed to accommodate this problem, or should this problem be fixed? I don't know how many other problems like this there are in the OPL. This is the only one I know of. It is also not a problem I use in my classes. Another instructor at my institution happens to use it.

[dpvc]

Perhaps this problem was just coded incorrectly, and only worked right mostly by chance.

Yes, I think that's true. But there is also precedent for Real() to be used to demote a real-like object (like fractions) to Real MathObjects. So it is not unreasonable to thing that Real() applied to a NumberWithUnits would return the numeric part. I suppose the real questions is wither that is useful or not, as I'm not sure what number would be best to return. Should it be the value in the units that the object has, or show it be converted to the value for the corresponding fundamental units, where you could reasonably compare values? I'm not sure what would be best. The contextUnits.pl implementation as Real($n) produce the number in the given units of the number-with-units $n, while $n->quantity gets you the value in fundamental units. One could implement that, I suppose, but the modifications needed to get Real() to work with the legacy NumberWithUnits is probably not worth it. Perhaps $n->value and $n->quantity would be sufficient? Then you would just need to implement the quantity method, which should be straight-forward.