tex, string, and separator options for units

[Alex-Jordan]

I'm revising the description for this in the post following this post. Leaving the original here for reference.

This implements one of the suggestions from @dpvc in #1437, and replaces that pull request.

• Units defined in Units.pm can have string, tex, string_separator, and tex_separator properties.
• All aliases for a unit will carry over these properties. To demonstrate what to do when this is unwanted, I moved micron to be a named unit instead of an alias.
• The string property is used when the unit or its aliases are printed as a string.
• The string property needs to use a string that can actually be printed in hardcopy. There is a comment about this in the file above where the big units hash is defined.
• Since a unit may be interpolated as part of an argument to Compute(), make sure that any all string properties are also unit names.
• The tex property is used when the unit or its aliases are printed as tex.
• The tex_separator property are only used when the unit is not part of a larger fractional unit, and the unit is the first unit following a number. Otherwise a space is used, as has been the case. For example, 180° and 180° s, but 180 s ° (except as tex).
• The string_separator property is similar, except it is also used even when there is a larger fractional unit. For example, 180°/s.
• Only deg is using tex_separator and string_separator, with each set to the empty string. I considered doing this with degF and degC, but Google tells me there should be a space for those units for scientific publications.
• contextUnits.pl was adding aliases for L (liter, liters, litre, and litres). It was cleaner to move those over to Units.pm. If there was a reason they were not already in Units.pm, I could move them back.

One thing that could still be improved, is that for string production, the spaces that are used in between numbers and units (when string_separator is not set), or between units and units should be nonbreaking spaces. I tried things that I though should work, but did not work for one reason or another. So that can be addressed in some future PR.

Try the problem from #1437 for some basic testing. Although you may want to also try with some more complicated units that have unit products and quotients.

[drgrice1]

This satisfies my initial request to make the properties centralized and defined in Units.pm, but will not work for what @dpvc asked. That is so that custom units can also take advantage of the new features. For this the addUnit method will need some more modification. Also the copies of the %context::Units::Context::DISPLAY hash made on line 958 for the context::Units::Unit package and line 1442 for the context::Units::NumberWithUnit package are going to be an issue for that as well. If the addUnit method modifies the %context::Units::Context::DISPLAY hash, then the copies will not be modified appropriately. Making those copies is also not particularly efficient. Those should be references instead.

[drgrice1]

Note that something is also needed at line 1856 in the checkMultDiv method of the context::Units::BOP package in order for the string_separator and tex_separator to work for student answers. Perhaps @dpvc can offer suggestions here.

[Alex-Jordan]

I'll put a pin in this (mark it as draft) until I come back from travel on July 7 (or later). I'll rework it. I can say that I did try at first to create a new %UNITS hash in Units.pm, which later in that same file was boiled down to %known_units to preserve backwards compatibility with things like parserNumberWithUnits.pl. And then I tried modifying contextUnits.pl accordingly to use the more structured %UNITS from Units.pm. That way the data for the new keys is really where it should be the whole time. I just couldn't manage to track all the added complications from the additional structure that this imposed on contextUnits.pl. But I'll try again.

One thing to think about in the meantime. I am leaning to not including the separator keys with this. This was really only included because of the degree symbol when the units are an angle. Not for anything else at present. So, like 60° instead of 60 °. That much would be fine. But what about 60°/s versus 60 °/s? What about if there is a product involving degrees, like 60° s versus 60 ° s? The current attempt makes these come out as 60°/s and 60° s, but more and more I am not sure that is good. So unless the team convinces me otherwise, when I redo this, I will leave out the separator features. If it's preferred, I could leave deg without having tex and string keys, so that deg stays as deg rather than automatically converting to °. Meanwhile authors who directly use ° will continue to see things like 60 °, so it will be a status quo thing, not a regression.

[drgrice1]

I think that the removal of the space for the degree symbol is more important to me than any of the other changes. But do what you can.

I think that 60°/s is correct, not 60 °/s. I am not sure what 60° s is, or even how exactly that would come about.

[dpvc]

I can say that I did try at first to create a new %UNITS hash in Units.pm, ... I just couldn't manage to track all the added complications from the additional structure that this imposed on contextUnits.pl.

Yeah, it's complicated. I've made a version of these two files that seem to do the trick. I'm attaching them here. One of the differences is that the %known_units has has the aliases as top-level keys, while%UNITS hash doesn't, so the aliases have to be handled a bit more carefully. I've refactored some of the code that deals with that in the units context.

units.tgz
contextUnits.tgz

See what you think.

[Alex-Jordan]

I've been modifying @dpvc's files, getting close to pushing a new version for all of this.

I've run into something that is an issue even before the changes happening here. The old Units.pm has % as a unit that is not usable with contextUnits.pl. I'm seeing at least three issues.

• Try to make Compute('1 %'); and you get Unexpected character '%'; see position 3 of formula at line 49 of [PG]/macros/core/Parser.pl. Something does not like trying to even parse %.
• There is no category in contextUnits for dimensionless units like percent. I guess we might want a new category for that. Unless there is something better, I propose `dimensionless'.
• Once % is parsed, I'll need to look closely at its string and TeX output.

I believe I can handle the latter two issues. But @dpvc, do you see how to address the first one?

Or maybe this is not going to work unless % becomes a new fundamental unit. I'm wondering if contexUnits.pl machinery can even work as expected if the unit is dimensionless.

[Alex-Jordan]

This is largely changes made by @dpvc in the files posted earlier in this thread. The purpose is that entries in Units.pm are now structured more. For example, what used to be:

ohm => {
		factor  =>  1,
		kg      =>  1,
		m       =>  2,
		amp     => -2,
		s       => -3,
		aliases => ["\x{2126}"]
	}

is now

ohm => {
		factor => 1,
		units  => {
			kg =>  1,
			m  =>  2,
			A  => -2,
			s  => -3,
		},
		prefixes => [qw(k M)],
		string   => "\x{2126}",
		TeX      => "\x{2126}",
		aliases  => ["\x{2126}"]
	}

• By structuring in the allowed prefixes, we no longer need separate units like kohm in the list.
• There are explicit string and TeX properties where helpful.
• The actual fundamental unit dimensions are a level deeper, to cleanly separate them from other properties.

All of this has been done in a way to preserve backward compatibility with parserNumberWithUnits.pl and parserFormulaWithUnits.pl. Basically the new %UNITS hash is converted to a hash that mostly matches the old hash. Then, contextUnits.pl has been updated to handle the new structure of %UNITS.

There are a few other changes.

• I did not like that amp was the fundamental unit and A was its alias. It was inconsistent with how other SI fundamental units are declared. So I switched those.
• microN was a declared unit, and I removed it. I could bring it back if we are worried that will break some problem somewhere. It seems unlikely.
• kWh was a declared unit, and I removed it. Because kW and h are already units.
• Added d for days.
• Added l for liters, along with prefixed cousins dl.
• Added liter, liters, litre, and litres.
• Added mL (since L gets the m prefix)
• Added mamp (since A is a fundamental unit, and amp is an alias, and since A gets the m prefix, and since all aliases that are 3 characters or less get the same prefixes)
• Added msec, nsec, usec for similar reasons.
• Added pc as the primary unit for parsec. Moved parsec to be an alias. Added parsecs as an alias. This was motivated by kpc and Mpc already being units, but not pc.
• Everywhere that u is used as a prefix, there will be an automatic alias with \x{00B5} as a prefix too.

There is also the noSeparator option. This applies to the degree symbol. I did not apply it to degF or degC (or degK) because Google suggests it is more professional to use spacing with those. Meanwhile noSeparator should also apply to %, but there are some issues to work out with % as described in my previous post.

When noSeparator is in effect, there will be no space following the number (magnitude) but only in certain conditions.

• string method, when there is no denominator unit or when there is only one numerator unit. So there is still a space for something weird like 5 (° m)/kg.
• TeX method, when there is no denominator unit. So while the string method might produce 5°/kg, the TeX method still has a space: 5\,\frac{°}{kg}

[Alex-Jordan]

A list of things I think could still be improved:

• The situation described earlier with %.
• The space between a magnitude and unit should really be nonbreaking space. But getting that to be the right thing for HTML versus PTX versus TeX output seems tricky.
• I suspect we could now have a shift or affine property that could be leveraged to allow comparisons between K quantities and degC quantities, etc. At least, for contextUnits,pl.
• As things are, the TeX method always wraps the TeX property in \text{...}. That happens to work for all of the TeX properties that currently exist. But if a TeX property used a math mode LaTeX macro, then I think that would fail. For example, \text{\mu{A}} would fail. (We are instead using \text{µA}.) But something using \text{...} or \mathrm{...} is needed for the units to come out upright.
• I think that 'B' (for byte) and 'b' (for bit) are worth consideration to be added as fundamental units. When prefixes are applied, there should be some thought put into whether a kB is officially 1000 bytes or 1024 bytes.
• We don't have a mechanism to deal with conflicts nicely. For example, c is already here as the speed of light. But c could be better used as the abbreviation for cup. There is also rad for radian, but also one rad means something else in the context of radiation dosages.
• lbf should mean a pound of force. In other words, a pound. But it is coded as if it stood for "foot pound", which is a unit of torque or energy. I am in favor of correcting this here, and let the problems that relied on this incorrect coding be corrected downstream.

[dpvc]

These look like great new features that you have added. Thanks for working on that.

I make some suggested changes below, and discuss an issue with modifying the %UNITS hash, and how that is problematic for the units context, that I think needs to be addressed.

[dpvc]

Try to make Compute('1 %'); and you get Unexpected character '%'; see position 3 of formula at line 49 of [PG]/macros/core/Parser.pl. Something does not like trying to even parse %.

Have you added the % unit to the context? The commands

loadMacros("contextUnits.pl");
Context("Units")->addUnits("%");
Compute("1 %");

works for me, both in the original contextUnits.pl, and the modified one.

It turns out, however, that you can't mix percentages with reals, so that

Compute('1 %) == .01

is false, and

`` perl
Compute('1 % + .01');

throws the error `Both operands of '+' must have units if one does; see position 5 of formula`,

while 

``` perl
Compute('1 %') + .01

throws Can't locate object method "classMatch" via package "0.01" (perhaps you forgot to load "0.01"?) at macros/contexts/contextUnits.pl line 1600.

That last error needs to be fixed, and I suspect the first two want to be fixed as well. There may be situations where you do want to force the use of '%' (like "What is .01 as a percentage?"), so there may need to be a flat to control that. It looks like the sameUnits flag might be reasonable for that.

But there are a number of other things that might need to be adjusted for percentages that I didn't think of earlier. For example 2% * 4% ends up as 8 %^2, and 10 / (2%) is 5/%. So there may need to be special rules for handling '%' (or "dimensionless" units in general). I'm not sure what to do with pure units involving %; currently, % % is %^2, for example.

I will think about how to address these issues, and let you know if I come up with anything.

[dpvc]

• The situation described earlier with %.

I'm working on that and think I have things pretty well worked out. I will post some updates as soon as they are ready.

• The space between a magnitude and unit should really be nonbreaking space. But getting that to be the right thing for HTML versus PTX versus TeX output seems tricky.

A \kern is non-breaking in TeX, so perhaps that could be used rather than \, and any of the spaces U+2000 to U+200B are non-breaking in HTML, I believe. Putting braces around the number, space, and unit would prevent inline line-breaks in TeX and MathJax. More aggressive would be to use \hbox{$...$} around the whole thing. Using U+00A0 in place of a space in the string output should prevent that from breaking.

• I suspect we could now have a shift or affine property that could be leveraged to allow comparisons between K quantities and degC quantities, etc. At least, for contextUnits.pl.

I was thinking the same thing.

• As things are, the TeX method always wraps the TeX property in \text{...}. That happens to work for all of the TeX properties that currently exist. But if a TeX property used a math mode LaTeX macro, then I think that would fail. For example, \text{\mu{A}} would fail. (We are instead using \text{µA}.) But something using\text{...}or \mathrm{...} is needed for the units to come out upright.

Well, one option would be to use TeX => '$\mu{A}$' so that you get \text{$\mu{A}$}, which would render properly. A bit awkward, but perhaps better than having to use \text{...} explicitly for everything else.

[Alex-Jordan]

First, a note that I pushed changes from your more minor comments. I agree with your more significant comments, just haven't gotten to addressing them.

Have you added the % unit to the context?

I can't reproduce what I thought was happening earlier, and my best guess is that I had a typo in "dimensionless" when I tried to load that new category of units.

About percents in general, if the exercise only involves percentages, then contextPercent.pl is great. The situations where treating % as a unit are good are when the answer is something like 2 %/s. Maybe that is all, in practice: relative growth rate questions.

I don't feel that adding a Real to a unit (even if it is %) needs to be supported. If you'd like to do that though, of course please do. On the other hand, if I see something like 2 + 1% in English, maybe the right interpretation for that is "2 + 1% (of 2)". So maybe the result would be 2.02, as opposed to 2.01. So I guess I lean toward having a controlled error message for this instead of it producing a result.

If we are in Units context, and we have Compute("2% * 2%"), I actually thing that 4 %^2 may be appropriate. The alternatives are (a) the Real number 0.0004, but now we are producing Reals form Units, which feels wrong. Or (b) the Unit 0.04 %. But now we have done something extra special with one of the 2% factors, converting it to a Real and then multiplying that by the other 2%. I don't know that the extra attention is worth it.

Question: radians are similar, in that they are arguably dimensionless. A radian is the ratio of a certain length (an arc length in meters, say) to a radius length (also in meters). But at one point long ago radian was blessed (not in the perl sense) as one of the fundamental units. Maybe % should be handled similarly.

Are there other dimensionless units? permille? ppm? Which units can you multiply by 1 m (for example) and get back something that should still be measured in meters? (note that would include radians.) Maybe all such units should be handled uniformly.

[dpvc]

Here are some possible changes to your contextUnits.pl file that deal with a number of issues that I found while working on the percent problem.

A bunch of the changes were to change "Can't" to "You can't" in the error messages, and to move some variable substitutions into %s specifiers for easier customization of the messages.

The add(), sub(), mult(), div(), and power() functions for NumberWithUnits are modified to handle raw formulas better (e.g., Compute("2m") + "x m"). This is done using a new checkFormulas() method. Also, add() and sub() are modified to avoid an error when adding or subtracting a number (e.g., Compute("2%") + 1).

The remainder of the changes are to handle the problems I saw with percentages. I had worked this out before seeing your last message, however. I discuss your comments below the code.

diff --git a/macros/contexts/contextUnits.pl b/macros/contexts/contextUnits.pl
index 78ce5ceb..990558bb 100644
--- a/macros/contexts/contextUnits.pl
+++ b/macros/contexts/contextUnits.pl
@@ -1004,7 +1004,7 @@ sub new {
                        if ($value->isConstant) {
                                $value = $value->eval;
                        } else {
-                               $value = $value->getTypicalValue($value)->unit;
+                               $value = $value->getTypicalValue($value);
                        }
                }
                return $value       if $value->type eq 'Unit';
@@ -1218,7 +1218,7 @@ sub mult {
        my ($ltype, $rtype) = ($l->type, $r->type);
        return $l->appendUnit($r) if $ltype eq 'Unit' && $rtype eq 'Unit';
        $self->Error("A Unit can't be multiplied by %s", Value::showClass($r)) if $ltype eq 'Unit';
-       $self->Error("Can't multiply %s by a Unit", Value::showClass($l))
+       $self->Error("You can't multiply %s by a Unit", Value::showClass($l))
                unless $ltype eq 'Number' || $ltype eq $context::Units::NUNIT;
        return $self->Package($context::Units::NUNIT)->new($l->copy, $r->copy);
 }
@@ -1233,7 +1233,7 @@ sub div {
        return $l->perUnit($r) if $ltype eq 'Unit' && $rtype eq 'Unit';
        return $self->Package($context::Units::NUNIT)->new($l->copy, $r->raiseUnit(-1, 1)) if $ltype eq 'Number';
        $self->Error("A Unit can't be divided by %s", Value::showClass($r))                if $ltype eq 'Unit';
-       $self->Error("Can't divide %s by a Unit", Value::showClass($l));
+       $self->Error("You can't divide %s by a Unit", Value::showClass($l));
 }
 
 #
@@ -1469,6 +1469,16 @@ sub new {
        $self->Error('Can\'t append a Unit to %s',  Value::showClass($n))    unless $n->type eq 'Number';
        $self->Error('Can\'t convert %s to a Unit', Value::showClass($unit)) unless $unit->classMatch('Unit');
        return $n if $unit->string eq '';
+       if ($unit->fString eq '' && $unit->string ne '%') {
+               if ($unit->{dunits}{'%'}) {
+                       $n /= 0.01 ** $unit->{dunits}{'%'};
+                       return $n;
+               }
+               if ($unit->{nunits}{'%'}) {
+                       $n *= 0.01 ** ($unit->{nunits}{'%'} - 1);
+                       $unit = $unit->new('%');
+               }
+       }
        return bless { data => [ $n, $unit ], context => $context, isConstant => 1 }, $class;
 }
 
@@ -1617,10 +1627,11 @@ sub abs {
 #
 sub add {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
-       shift;
-       ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
+       ($l, $r) = ($self->makeValue($l), $self->makeValue($r));
+       my $f = $self->checkFormulas($l, $r, $_[2], sub { $_[0] + $_[1] });
+       return $f if defined $f;
        $self->Error('You can\'t add %s to %s', $l->showClass, $r->showClass)
-               unless $other->classMatch('NumberWithUnit');
+               unless ($_[2] ? $l : $r)->classMatch('NumberWithUnit');
        $self->Error('You can only add quantities with the same units') unless $l->fString eq $r->fString;
        return $self->new($l->number + $r->quantity / $l->factor, $l->unit->copy);
 }
@@ -1630,10 +1641,11 @@ sub add {
 #
 sub sub {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
-       shift;
-       ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
+       ($l, $r) = ($self->makeValue($l), $self->makeValue($r));
+       my $f = $self->checkFormulas($l, $r, $_[2], sub { $_[0] - $_[1] });
+       return $f if defined $f;
        $self->Error('You can\'t subtract %s from %s', $r->showClass, $l->showClass)
-               unless $other->classMatch('NumberWithUnit');
+               unless ($_[2] ? $l : $r)->classMatch('NumberWithUnit');
        $self->Error('You can only subtract quantities with the same units') unless $l->fString eq $r->fString;
        return $self->new($l->number - $r->quantity / $l->factor, $l->unit->copy);
 }
@@ -1644,14 +1656,16 @@ sub sub {
 sub mult {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
        ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
-       my ($lUnit, $rUnit)   = ($l->classMatch('Unit'), $r->classMatch('Unit'));
+       my $f = $self->checkFormulas($l, $r, $_[2], sub { $_[0] * $_[1] });
+       return $f if defined $f;
+       my ($lUnit,  $rUnit)  = ($l->classMatch('Unit'),           $r->classMatch('Unit'));
        my ($lUnitN, $rUnitN) = ($l->classMatch('NumberWithUnit'), $r->classMatch('NumberWithUnit'));
        return $self->new($l->number->copy,        $l->unit->appendUnit($r))       if $lUnitN && $rUnit;
        return $self->new($l->number * $r->number, $l->unit->appendUnit($r->unit)) if $lUnitN && $rUnitN;
        return $self->new($l * $r->number,         $r->unit->copy)                 if $l->type eq 'Number';
        return $self->new($l->number * $r,         $l->unit->copy)                 if $r->type eq 'Number';
        $self->Error("A Unit can't be multiplied by %s", Value::showClass($r)) if $lUnit;
-       $self->Error("Can't multiply %s by a Unit", Value::showClass($l));
+       $self->Error("You can't multiply %s by a Unit", Value::showClass($l));
 }
 
 #
@@ -1661,14 +1675,16 @@ sub mult {
 sub div {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
        ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
-       my ($lUnit, $rUnit)   = ($l->classMatch('Unit'), $r->classMatch('Unit'));
+       my $f = $self->checkFormulas($l, $r, $_[2], sub { $_[0] / $_[1] });
+       return $f if defined $f;
+       my ($lUnit,  $rUnit)  = ($l->classMatch('Unit'),           $r->classMatch('Unit'));
        my ($lUnitN, $rUnitN) = ($l->classMatch('NumberWithUnit'), $r->classMatch('NumberWithUnit'));
        return $self->new($l->number->copy,        $l->unit->perUnit($r))       if $lUnitN && $rUnit;
        return $self->new($l->number / $r->number, $l->unit->perUnit($r->unit)) if $lUnitN && $rUnitN;
        return $self->new($l / $r->number,         $r->unit->raiseUnit(-1, 1))  if $l->type eq 'Number';
        return $self->new($l->number / $r,         $l->unit->copy)              if $r->type eq 'Number';
        $self->Error("A Unit can't be divided by %s", Value::showClass($r)) if $lUnit;
-       $self->Error("Can't divide %s by a Unit", Value::showClass($l));
+       $self->Error("You can't divide %s by a Unit", Value::showClass($l));
 }
 
 #
@@ -1677,10 +1693,12 @@ sub div {
 sub power {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
        ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
-       $self->Error("A $context::Units::NUNIT can't be raised to %s", $r->showClass)
+       my $f = $self->checkFormulas($l, $r, $_[2], sub { $_[0]**$_[1] });
+       return $f if defined $f;
+       $self->Error("A %s can't be raised to %s", $context::Units::NUNIT, $r->showClass)
                unless $l->classMatch('NumberWithUnit') && $r->type eq 'Number';
        my $n = $r->value;
-       $self->Error("A $context::Units::NUNIT can only be raised to a non-zero integer value")
+       $self->Error("A %s can only be raised to a non-zero integer value", $context::Units::NUNIT)
                if $n == 0 || CORE::int($n) != $n;
        return $self->new($l->number**$n, $l->unit->raiseUnit($n));
 }
@@ -1690,7 +1708,8 @@ sub power {
 #
 sub compare {
        my ($self, $l, $r, $other) = Value::checkOpOrder(@_);
-       ($l, $r) = (Value::makeValue($l), Value::makeValue($r));
+       ($l, $r) = ($self->makeValue($l), $self->makeValue($r));
+       return ($_[2] ? 1 : -1) if ($_[2] ? $l : $r)->isFormula;
        return $l->type eq 'Unit' || $r->classMatch('NumberWithUnit') ? -1 : 1 unless $l->type eq $r->type;
        my ($ls, $rs) = ($l->fString, $r->fString);
        return $ls cmp $rs unless $ls eq $rs;
@@ -1705,6 +1724,30 @@ sub D {
        return $self->new($self->number->D(@_), $self->unit->D(@_));
 }
 
+#
+#  Convert to Value objects, taking percents into account
+#
+sub makeValue {
+       my $self = shift;
+       my $x    = Value::makeValue(shift);
+       return
+               $x->isReal
+               && $self->fString eq ''
+               && $self->unit->string eq '%'
+               && !$self->getFlag('sameUnits') ? $self->new($x * 100, '%') : $x;
+}
+
+#
+#  Check if operands are formulas and if so, convert the other to a
+#  formula and perform the operation.
+#
+sub checkFormulas {
+       my ($self, $l, $r, $switch, $fn) = @_;
+       return unless ($switch ? $l : $r)->isFormula;
+       ($switch ? $r : $l) = $self->Package('Formula')->new($self);
+       return &$fn($l, $r);
+}
+
 #############################################################
 
 #
@@ -1822,6 +1865,16 @@ sub hasNumberUnitOperands {
        return $ltype eq $context::Units::NUNIT && $rtype eq $context::Units::NUNIT;
 }
 
+#
+#  Check if one operand is a percentage and the other is a number
+#
+sub hasPercentNumberOperands {
+       my ($self, $l, $r, $ltype, $rtype) = @_;
+       my $NUNIT = $context::Units::NUNIT;
+       return ($ltype eq $NUNIT && $rtype eq 'Number' && $self->Package('Formula')->new($l)->unit->string eq '%')
+               || ($rtype eq $NUNIT && $ltype eq 'Number' && $self->Package('Formula')->new($r)->unit->string eq '%');
+}
+
 #
 #  Call the _check from the original class unless one of the operands
 #  is a Number with Units, in which case, we check that operations are
@@ -1829,17 +1882,19 @@ sub hasNumberUnitOperands {
 #
 sub checkNumberUnits {
        my $self = shift;
-       my ($ltype, $rtype) = ($self->{lop}->type, $self->{rop}->type);
+       my ($l, $r) = ($self->{lop}, $self->{rop});
+       my ($ltype, $rtype) = ($l->type, $r->type);
        return $self->mutate->_check unless $self->hasNumberUnitOperand($ltype, $rtype);
+       $self->{type} = $context::Units::NUMBER_WITH_UNIT;
+       return if $self->hasPercentNumberOperands($l, $r, $ltype, $rtype);
        $self->Error("Both operands of '%s' must have units if one does", $self->{bop})
                unless $self->hasNumberUnitOperands($ltype, $rtype);
-       my $lunit = $self->Package('Formula')->new($self->{lop})->unit;
-       my $runit = $self->Package('Formula')->new($self->{rop})->unit;
+       my $lunit = $self->Package('Formula')->new($l)->unit;
+       my $runit = $self->Package('Formula')->new($r)->unit;
        $self->Error("Units '%s' and '%s' are not compatible", $lunit->string, $runit->string)
                unless $lunit->fString eq $runit->fString;
-       $self->Error("Can't use '%s' with Numbers with Units in this context", $self->{bop})
+       $self->Error("You can't use '%s' with Numbers with Units in this context", $self->{bop})
                if $self->context->flag('limitedOperators');
-       $self->{type} = $context::Units::NUMBER_WITH_UNIT;
        $self->factorUnits if !$self->{isConstant} && $self->context->flag('factorUnits');
 }
 
@@ -1860,7 +1915,7 @@ sub checkMultDiv {
                || $self->bothUnitOperands($ltype, $rtype)
                || ($ltype eq 'Number' && $rtype eq 'Unit');
        return $self->mutate->_check unless $self->hasUnitOperand($ltype, $rtype);
-       $self->Error("Can't $op1 two Numbers with Units in this context")
+       $self->Error("You can't %s two Numbers with Units in this context", $op1)
                if $self->context->flag('limitedOperators') && $ltype eq $context::Units::NUNIT && $ltype eq $rtype;
        $self->{def} = {
                %{ $self->{def} },
@@ -1878,7 +1933,7 @@ sub checkMultDiv {
        return
                if ($ltype eq 'Number' && $rtype eq $context::Units::NUNIT)
                || ($rtype eq 'Number' && $ltype eq $context::Units::NUNIT && $mult);
-       $self->Error('A %s can only be $op2 by a Unit', $ltype) if $lHasUnit;
+       $self->Error('A %s can only be %s by a Unit', $ltype, $op2) if $lHasUnit;
        $self->Error('A Unit can only $action another Unit') unless $ltype eq 'Number' || $mult;
 }
 
@@ -2041,11 +2096,11 @@ sub _check {
        return $self->mutate->_check
                unless ($ltype eq 'Unit' || $ltype eq $context::Units::NUNIT) && $rtype eq 'Number';
        if ($self->context->flag('limitedOperators')) {
-               $self->Error("Can't raise a %s to a power in this context", $ltype) if $ltype ne 'Unit';
+               $self->Error("You can't raise a %s to a power in this context", $ltype) if $ltype ne 'Unit';
                my $unit   = $self->{lop}->eval;
                my @nunits = keys %{ $unit->{nunits} };
                my @dunits = keys %{ $unit->{dunits} };
-               $self->Error("Can't raise a Compound Unit to a power in this context") unless @nunits == 1 && @dunits == 0;
+               $self->Error("You can't raise a Compound Unit to a power in this context") unless @nunits == 1 && @dunits == 0;
        }
        $self->{type} = $self->{lop}->{type};
 }
@@ -2223,7 +2278,7 @@ our @ISA = ('context::Units::Super', 'Value::Formula');
 
 sub checkNumberWithUnits {
        my ($self, $method) = @_;
-       $self->Error("Can't use '->$method' with " . $self->showClass)
+       $self->Error([ "You can't use '->%s' with %s", $method, $self->showClass ])
                unless $self->type eq $context::Units::NUNIT;
 }
 
@@ -2269,7 +2324,7 @@ our @ISA = qw(Parser::List);
 sub _check {
        my $self = shift;
        $self->{type}{list} = 0;
-       $self->Error("Lists of units are not allowed") if ($self->{type}{length} != 1);
+       $self->Error("Lists of units are not allowed") if $self->{type}{length} != 1;
        my $arg = $self->{coords}[0];
        $self->Error("Parentheses should only be used around units in this context")
                unless $arg->type eq 'Unit' || $self->context->flag("allowBadOperands");

---

If I see something like 2 + 1% in English, maybe the right interpretation for that is "2 + 1% (of 2)".

I understand the thought, but I think that introduces some problems. For example, does 1% + 2 mean the same thing? If not, then that would make + no longer be commutative, and that would have consequences for reduction rules (like the ones that switch -x + y to y - x). Also, contextPercent.pl treats 2 + 1% as 2 + .01, so making different in the units context would be inconsistent.

What I have in mind is dealing with questions like If 2% of x is 32, then x = [___], where the student could use 32/(2%) to get the result. Currently, this would give 16/%, which is a pretty strange answer. The code above turns this into the real number 1600.

Similarly, I would find (2%) * (2%) being 4%^2 to be rather confusing. You say

The alternatives are (a) the Real number 0.0004, but now we are producing Reals form Units, which feels wrong. Or (b) the Unit 0.04 %.

and I considered both of these. Either works better for me, and the code above does (b), but could easily produce (a) instead. (That is all handled in the first large green section above, at 1469. The second if could be handled in the same way as the first one.)

You are concerned that this produces Reals from Units, but that is already done in some cases. For example, 10m/(2m) produces the Real 5 with no units. So there is already precedent for having some expressions produce non-units.

I don't know that the extra attention is worth it.

It's really only that one block of code that does that.

As for 2% + 1, that is handled by a new makeValue() method that is used in the add() and sub() methods (and can be disabled by using the sameUnits => 1 flag, though a new flag could be added for this instead). It promotes a real to a percentage (so that adding and subtracting with have both be percentages). If you don't want that to happen, change $self->makeValue(...) to Value::makeValue(...) in add() and sub() again. You will need to keep the new method, however, as compare() also uses it, so that you can compare 2% to 0.02 and have them be equal (unless sameUnits is true). I assume that is expected, right?

There is also a new hasPercentNumberOperands() method used when parsing formulas to allow addition and subtraction of numeric quantities and percentages. E.g., Compute("3% + x") similarly to Compute("3% + 5"). If you don't want to support adding reals to percentages, then you can remove that new method and the line that calls it.

Question: radians are similar, in that they are arguably dimensionless.

I hadn't thought about that, but I guess that does seem to be similar. The code I have is certainly specific to '%', but could be extended to handle other similar units, like radians. In that case, I would not call the category dimensionless, but rather pertentages, since it doesn't include rad.

Which units can you multiply by 1 m (for example) and get back something that should still be measured in meters?

Well, we currently don't have any support for that. I suppose 100m * (2%) should be 2m rather than 200 m %. I'll have to think about how that could be accomplished most easily. And if 10m * (3 rad) should be 30 m rather than 30 m rad then we need to do something to make that happen.

[dpvc]

We don't have a mechanism to deal with conflicts nicely. For example, c is already here as the speed of light. But c could be better used as the abbreviation for cup.

Well, I suppose the new %UNITS hash could be configured as a hash of categories (which could simplify the construction of categories in the units context, if they corresponded properly). For example, you could have

%UNITS = (
	fundamental => {
		...
	},
	angles => {
		...
	},
	time => {
		...
	},
	...
);

so that c could appear in two of the categories and mean different things. Of course, when constructing the %known_units hash, one of the two would have to take precedence, For that, there could be a precedence field in the duplicates that would tell which one should be used in preference to the other.

It would also be possible to have these categories include a default set of fundamental units, so that the units don't have to be repeated for every entry in the category, but could be overridden if needed in a specific entry in the category. Something like

%UNITS = {
	...
	velocity => {
		default_units => {
			s => -1,
			m => 1
		},
		definitions => {
			knots => { factor => 0.5144444444 },
			c     => { factor => 299792458 },
			mph   => { factor => 0.44704 },
		}
	},
	...
	magnetism => {
		default_units => {
			s  => -2,
			kg =>  1,
			A  => -1
		},
		definitions => {
			T => {
				factor => 1,
 				prefixes => [qw(m)]
			},
			G => {
				factor => 1E-4,
			},
			Wb => {
				factor => 1,
				units  => {
					s  => -2,
					m  =>  2,
					kg =>  1,
					A  => -1
				}
			},
			H => {
				factor => 1,
				units  => {
					s  => -2,
					m  =>  2,
					kg =>  1,
					A  => -2
				}
			}
		}
	},
	...
};

Such an arrangement could simplify the handling of categories considerably in contextUnits.pl. Now seems to be the time to consider such changes, as we add the new %UNITS hash.

[Alex-Jordan]

Thanks @dpvc, I tried your changes and they all worked as described. So I applied the diff.

Everything you wrote after the diff sounds good too. There's not much time left for this release. I believe @drgrice1 is planning to trigger the 2.21 release early next week. I know that I don't have much available time to try to implement some of the things you brought up. Do you have an opinion on which is best?

1. Leave this PR out of the release, and make it better for 2.22 next time?
2. Merge this now in time for the release (assuming two people can review and approve it) and make improvements later.
3. Try to rush in the things you brought up at the end?

I'm trying to get a handle on what might get too messy once more problem authors have invested in using contextUnits.pl, especially when it comes to adding custom units.

[dpvc]

I think 2 is probably the best option. you have done important work here, and it should be in 2.21. It can be expanded later to include the other ideas. The changes that I suggested to %UNITS are really behind-the-scenes changes, so should not affect problems being written. The issues around % and rad already exists, so making your changes should not really impact the fact that future changes might affect current usage. We can make things as backward compatible as possible, and fix the rest.

[drgrice1]

I agree with @dpvc. Lets get the primary changes into 2.21, and more changes can be made later.

The release will not be until the first week of August.

[Alex-Jordan]

I worked back and forth with Claude to add the most recent commit. It's kept as a separate commit in case it is determined that this Clause usage has overcomplicated things.

Mainly, this last commit restructures %UNITS (in Units.pm so that the keys are categories ('length', etc). Then contextUnits.pl's category handling is simplified to mostly fall back on the categories from %UNITS. It adds metric-lengths and imperial-lengths categories, and it handles the fundamental units category separately. There are several new subroutines needed to handle the new structure of %UNITS.

Now we can have c stand for one cup in the "volume" category, without it clashing with c for the speed of light. We have to flag the "cup" c as something to exclude from the construction of the legacy %known_units hash.

A lot of commentary is added to explain some things, but it is not entirely an artifact of using Claude. I reviewed the code commentary and kept what I thought was good, given the complicated stuff happening here. I did remove a bunch that Claude wrote. This is overall about 225 fewer lines of code, even counting commentary.

Lastly, I added some utilities for authors: listCategories() and listUnitsFor() let an author see the list of available categories and the list of units that come with a given category. This way the POD in contextUnits.pl doesn't need to be kept in sync with Units.pm where almost all of the categories are actually defined.

[dpvc]

This all looks good. I think these changes really improve the unit handling a lot. Thanks for making them.

I have one suggested change concerning the unitCategories flag.

[dpvc]

Note that the keys here will be in a random order, so the order in which the categories are handled if there is more than one will be different each time. That means if you load two categories that both define the same unit (like your example of c in volume and velocity), the category that is processed last will be the one that is used, but you don't know which that will be.

I think it would be better to use an array rather than a hash for unitCategories so that the order is always the same. That means you can decide which c to use by putting its category last. Since you have removed the no_legacy values, there doesn't seem to be another way to decide which to use. (I had originally suggested a priority value so that if several categories defined the same unit name, you could take the highest priority one from among the categories selected. But letting the author indicate it by the order of the categories selected is probably better.)

Also, I don't think this should be a flag, but rather an actual value in the context itself. Flags are meant to be user settings, not internal state. Unfortunately, there is a bug in pg/lib/Value/Context.pm that would prevent the array from being copied if the context is copied, so leaving the array in the flags is probably reasonable for now. It can be moved later if necessary.

[Alex-Jordan]

I changed this to be an array. Even if no_legacy were present, there might be two units in the future that are both no_legacy, so using the author's ordering is better.

About it being a flag. This was part of the changes when I asked Claude for help. It told me that this would need to be a flag for precisely the reason you are saying, so that it would be copied over.

[drgrice1]

I am seeing an issue with the following problem:

DOCUMENT();

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

Context('Units')->withUnitsFor('time', 'mass', 'length', 'force');

$ansUnit1 = NumberWithUnits('34',   'g');
$ansUnit2 = NumberWithUnits('23.5', 'ft/s^2');
$ansUnit3 = NumberWithUnits('5',    'N*m/s');

BEGIN_PGML
[`[$ansUnit1] =`] [_]{$ansUnit1}{10}

[`[$ansUnit2] =`] [_]{$ansUnit2}{10}

[`[$ansUnit3] =`] [_]{$ansUnit3}{10}
END_PGML

ENDDOCUMENT();

With the PG-2.21 branch, this works as expected. However, with this pull request there are several warnings about the units lb, pounds, pound, tons, and ton already existing. The problem seems to work as expected other than the warnings.

[dpvc]

In case you are interested, the problem with the Context.pm is that when copying a context, the part that handles various types of sub-objects looks at a default new context for the data about what to copy rather than the original context. That means if the context added items to be copied, they won't be. Argh! That has been there for 20 years!

diff --git a/lib/Value/Context.pm b/lib/Value/Context.pm
index 6650b147..9e8841fc 100644
--- a/lib/Value/Context.pm
+++ b/lib/Value/Context.pm
@@ -69,22 +69,22 @@ sub copy {
        my $self    = shift;
        my $context = $self->new();
        $context->{_initialized} = 0;
-       foreach my $data (@{ $context->{data}{objects} }) {
+       foreach my $data (@{ $self->{data}{objects} }) {
                $context->{$data}->copy($self->{$data});
        }
-       foreach my $data (@{ $context->{data}{hashes} }) {
+       foreach my $data (@{ $self->{data}{hashes} }) {
                $context->{$data} = {};
                foreach my $x (keys %{ $self->{$data} }) {
                        $context->{$data}{$x} = { %{ $self->{$data}{$x} } };
                }
        }
-       foreach my $data (@{ $context->{data}{arrays} }) {
+       foreach my $data (@{ $self->{data}{arrays} }) {
                $context->{$data} = {};
                foreach my $x (keys %{ $self->{$data} }) {
                        $context->{$data}{$x} = [ @{ $self->{$data}{$x} } ];
                }
        }
-       foreach my $data (@{ $context->{data}{values} }) {
+       foreach my $data (@{ $self->{data}{values} }) {
                $context->{$data} = { %{ $self->{$data} } };
        }
        $context->{error}{msg}     = { %{ $self->{error}{msg} } };

Fixing this would also you to add something like

$context->{units}{categories} = [];
push(@{$context->{data}{arrays}}, 'units');

rather than using the flags. But without this change, the categories would not be copied if you copied an existing units context.

[Alex-Jordan]

In case you are interested

Certainly interested. I think this will have to wait until the next release though. I believe we are trying to release today. Even the changes here might not make it unless I can investigate and repair the issue @drgrice1 found. And even then, I'm not sure people have time to test.

[Alex-Jordan]

I am seeing an issue with the following problem:

We have to decide if we actually want those warnings. They may be a good thing, alerting the author to the ambiguity. In your problem, you loaded the mass category, which has a "ton" of mass (907.18474 kg).

Then you loaded the "force" category, which has a "ton" of force (8896.44323 N). In the end, this second one survives. So Compute("1 ton") == Compute("8896 N") is true, but Compute("1 ton") == Compute("907 kg") is false.

This may be a situation where it is good to bring the conflict to the author's attention. They can get what they want by changing order of categories. And/or removing units one at a time, then adding them one at a time. (They can add the particular "ton" they want using addUnits(ton => "force") or addUnits(ton => "mass").)

[drgrice1]

If the objective is to not restrict the student to entering specific units, but allow any unit to be used as long as it is correct in the given unit, then you would not want to add each unit individually. Adding units by category is convenient for this, and it is nice to be able to take care of that in one call as in that problem. That does work without this pull request, and so that is a change in behavior that we will have to decide on.

[Alex-Jordan]

I'm not following your last comment. You can add categories before and after this. You can remove or add individual units as well before or after this.

If you have

Context('Units')->withUnitsFor('time', 'mass', 'length', 'force');

and you want lb and ton to be force units, leave it as it is. If you want them to be mass units, swap the order you load them. Or if you want them as force units but don't want the errors, you can do like:

Context('Units')->withUnitsFor('time', 'mass', 'length');
Context()->removeUnits('ton', 'lb');
Context()->addUnitCategory('force');

One thing that is here though is that I've grown the %UNITS hash to hold more units. For example, the new "lb" as a unit of mass in addition to the old unit of force. Or "c" could be a cup of volume, not just the old speed of light. And there are completely new units too, like ppm, grad, yd, and more. Their presence could change problem behavior.

[dpvc]

Note that all the unit adding/removing commands return the context so that you can chain them together. That is:

Context('Units')
  ->withUnitsFor('time', 'mass', 'length')
  ->removeUnits('ton', 'lb')
  ->addUnitCategory('force');

Alternatively, you could add code to remove the previous definition (if there is one) before adding the new one in the two places where units or aliases are added to the constants. That would avoid the error, and enforce the "last definition gets used" paradigm.

[Alex-Jordan]

you can chain them together.

That doesn't work for me. I haven't investigated, but it feels like one of the subroutines is making the context alterations but not returning the context to apply the next method to.

[Alex-Jordan]

That would avoid the error, and enforce the "last definition gets used" paradigm.

But I think that's the question. Don't we actually want this warning to happen? It's a warning, not an error. And it alerts the author to something they may not be thinking about. They may not be aware that "ton" is in two categories until this kind of warning happens.

[dpvc]

That doesn't work for me.

OK, I had looked through the code to see that the functions did return $self before commenting (but didn't actually run it — always a bad idea). It looks like it might be removeUnits() that is the culprit; I missed the for clause at the end of the last line, which changes the return value. It should probably get a return $self;.

[dpvc]

Don't we actually want this warning to happen?

Up to you. I'm in favor of messages for issues like this, but there seemed to be concern about whether it would affect existing problems that are "working". I put that in quotes because they really aren't working, they just never told you that they were (potentially) broken so you didn't know. That is, they had a clash of names that went unnoticed, and so may or may not have actually been working as intended. It may just have been a fluke that the last one was the one that was wanted, or it may have been that the other one was the intended meaning and it was never actually tested. But I agree with you, it is better to be told there is a potential problem.

[Alex-Jordan]

OK, it should be easy to fix the chained methods.

I don't think there would be a clash before this PR, because this is where we are introducing duplicated unit names.

[drgrice1]

I think lets merge this, and fix as needed with hotfixes.