Make Fraction contexts as extensions so you can add fractions to other compatible contexts.

[dpvc]

This PR modifies the contextFraction.pl file to use the context-extension framework, making it possible to add fractions to any context. It still creates the original four fraction contexts, but also provides context::Fraction::extending() that can be used to add fractions to other contexts. E.g., you could use context::Fraction::extending("Matrix") to get a context in which fractions can be entered in matrices.

This should be backward compatible with the current contextFraction.pl from an author's perspective, except for the following:

• The strictMultiplication and strictMinus flags have been removed, as they are no longer needed (the strict handling is done by extending the LimitedNumeric context, rather than reimplementing the strict behavior).
• The handling of the internal information about integer values, negative values, and fractions has been modified. In the past, the object's class was modified to return INTEGER, MINUS and FRACTION, but now that fractions can be attached to arbitrary contexts, we don't want to modify the class of numbers, since that could affect how the other context operates. Instead these values are stored internally in private data within the typeRef objects.

It appears that at least one problem (Contrib/PCC/BasicAlgebra/LinearEquationApplications/Ratio20.pg) subclasses the LimitedFraction class and uses the original class values, so would need to be modified to work with the new contexts. I have renamed the original contextFraction.pl as legacyFraction.pl, for use in cases like this, though you all can decide if that is necessary or not.

Finally, I modified the t/contexts/fraction.t file to include the needed Parser classes for the new contextFractions.pl. Of course, that test file doesn't test most of the functionality, and needs many more tests to fully test the various options.

This PR includes the contextExtensions.pl file, even though I made a separate PR for it. Because that branch isn't in the openwebwork/pg repository, I can't target this PR to that branch. When that PR is merged, the contextExtensions.pl file should disappear from here.

[dpvc]

PS, because this contextFraction.pl file is substantially different from the original, it may help to just view the file directly, rather than try to navigate the diff.

[drgrice1]

I am seeing an issue with the following MWE:

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

Context('Fraction');
$ans = Compute('x/7');

BEGIN_PGML
[`[$ans] =`] [_]{$ans}{10}
END_PGML
ENDDOCUMENT();

That works with the develop branch, but gives an error with this pull request.

[dpvc]

OK, I forgot to include the cmp_defaults method. I was pretty sure I had tested that, but I guess I misremembered (probably remembered testing the units contexts instead). I've pushed the fix.

[drgrice1]

That fixes the issue with that example. Thanks.

I am seeing another issue when different contexts are used as in the following:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'contextFraction.pl', 'PGcourse.pl');
Context('Fraction');
$fractionAns = Compute('x/7');
Context('Numeric');
$numericAns = Compute("3");
BEGIN_PGML
[`[$fractionAns] =`] [_]{$fractionAns}{10}

[`[$numericAns] = `] [_]{$numericAns}{10}
END_PGML
ENDDOCUMENT();

[drgrice1]

Note that if either answer is removed from that problem it works. Also, if the fraction does not have x in it, then there is no problem.

[drgrice1]

You latest changes fix the issues! Thanks.

[drgrice1]

You may not like the perltidy format, but look at much of the formatting of webwork and PG code before we adopted it. It was utterly horrendous. If you set your editor up with it, it also makes it easy to have nicely formatted code with no effort.

[dpvc]

You latest changes fix the issues! Thanks.

Yes, I was still doing some more changes before writing up the details.

It turns out that changing the context was what was causing the problem. Although MathObjects contain a pointer to the context that created them, there are places where new objects are created via their new() or make() methods where they are passed the context, since the $self reference is the actual class name, not an instance, in those cases. That means those new() and make() calls don't have $self->context set to the context where the original object was created, but rather it defaults to the current context. Since the extension data is stored in the original object's context, if the context has changed, these new() and make() calls need to use the passed context in order to get the extension information. This wasn't taken into account in the context::Extensions::Super class, and so I had to include passing the context to these in some instances in order to get it to work when the context has been changed, as in your example.

I need to update the units PR to accommodate those changes as well.

You may not like the perltidy format

I don't object to it in principle, but I think some of the internal alignment make things harder to read, and some of the spacing is unnecessary. For example, just because two consecutive lines have equal signs or suffix if statements doesn't mean those equal signs or ifs should be lined up. For example, this

	$x = $context->Package("Formula")->new($context, $x)->eval if !ref($x) && $x =~ m!/!;
	$x = $x->eval                                              if @_ == 0  && Value::classMatch($x, 'Fraction');

is nonsense in my opinion. It is much harder to see the second line as being a conditional when the if is so far away from the assignment. I also don't care for the extra spacing within braces, parens, and brackets for single-line expressions, like the spaces added in the brackets here, though I know that is a popular approach:

	my $class = join('::', (split(/::/, ref($self) || $self))[ 0, 1 ]);

That was the source of my commit comment. I frequently end up shaking my head at some of the changes it makes. But I understand the desire to have consistency, and you all can decide on the formatting you want to use. I may still grumble about it at times, however.

[drgrice1]

I also don't like some of the formatting at times. In some cases we may be able to improve things by tweaking the settings. At least you can do that, unlike prettier for JavaScript, typescript, etc. I just know that some of the webwork and PG code used to be really untradable with excessively inconsistent indentation in particular.

[drgrice1]

The particular alignment issue you mentioned is certainly not the nicest in appearance, I agree.

[dpvc]

I agree with you about the consistency for indenting, line breaks, and continuation lines; that is a big win. It is the internal alignment that I find so unsettling. It seems to me that internal alignment should be used to indicate a meaningful structural connection between two lines, but most of the internal alignment that I see is meaningless, like the one I illustrated above (and I did choose it because of its particularly grievous nature, though most aren't that bad). That has several consequences. First, it makes things harder to read because the extra spacing interferes with reading the individual line, and one is incorrectly lead to believe there is a connection to a previous or following line, and it takes time to recognize that that is not the case. Second, because that is so frequently not the case, it trains you to ignore the alignment, as it is generally is meaningless. That means you have watered down the meaning of alignment, so that when it truly is important, you are less likely to recognize it among the sea of other nonsense alignments. For the most part, I think internal alignment should be relatively rare, and reserved for cases where a connection needs to be emphasized. I'm happy with alignment of => arrows in multi-line hash definitions, and even alignment of equal signs during the initialization of variables can be useful, but not every consecutive equal sign should be aligned with a previous one, and aligning && in the if statements like done in my example above is generally unnecessary and should only be done when the structure of the two really is connected. Similarly, aligning entries in two consecutive array literals is probably not called for except when the two arrays are connected in some way.

My own preference would be to have the authors do the internal alignment that is important (since they are the ones that understand which ones have meaning, whereas perltidy clearly doesn't), and have a style guide that indicates the ones that you expect to make, and request that authors abide by that before merging if they don't. By forcing such alignments, you may be prevention poor coders from making bad decisions, but you are also forcing other bad decisions, and preventing good coders from providing meaningful alignments (and actively undoing the meaningful ones they include). That seems to be to be guaranteeing a mediocre output; certainly not as bad as it was, but not as good as it could be.

Again, I realize it is up to the currently active programming community (of which I am not longer a member) to decide these issues, and I will abide by the choices that are made. But I reserve the right to make snarky commit comments now and again. :-)

[dpvc]

I should point out that my recent changes to handle your context-switching example included a minor change to lib/Value.pm. It turns out that

my $symbol = shift || '';

would perform stringification on the first argument in order to do the || operation, and that caused a problem in the Value::isContext() function in some situations. So this has been updated to use // instead of ||. That should not be a controversial change.

[dpvc]

PS, I meant to thank you for the careful testing. Very helpful!

[dpvc]

From #1106:

I was experimenting with using this in some problems that I have that already sort of do this. In those problems I had previously used Context('Fraction') and then called Context()->parens->set('(' => { type => 'Point' }). That worked for my purposes. Of course with this, calling Context(context::Fraction::extending('Point')) is probably better.

Anyway, one thing that frequently occurs on this problem is that when a coordinate of the point is supposed to be 5/2, a student will answer 2 1/2. I generally don't approve of mixed number usage for coordinates of points, but I guess it is correct. So I experimented with this to see if simply adding the allowMixedNumbers => 1 flag would work. Upon entering (2 1/2, -1) for the answer, I get the message "Coordinate of Point can't be a Fraction", so it doesn't work as I had thought it would. It seems that the signedNumber pattern is the problem, and perhaps should be extended in the case that the allowMixedNumbers flag is set to include mixed numbers.

This turns out to be due to a change included in the new contextFraction.pl that preserves the fact that a student has entered a mixed number and stringifies their answer the same way that they entered it. So 2 1/2 will show as 2 1/2 rather than 5/2 if that is how they entered it. That affects the Value::isNumber() check when the student enters 2 1/2 since it no longer matches that signedNumber pattern, as you point out.

Unfortunately, there is no easy way to adjust the signedNumber pattern based on the flag, which can be set in the context, or in the cmp() call, or on the fraction object itself. I think the Value::isNumber() function is where the change should be made. The current code is:

pg/lib/Value.pm

Lines 321 to 325 in 10746b1

	sub isNumber {
	my $n = shift;
	return $n->{tree}->isNumber if isFormula($n);
	return classMatch($n, 'Real', 'Complex') || matchNumber($n);
	}

There are several approaches to resolving this. One would be to add a check like

sub isNumber { 
 	my $n = shift; 
 	return $n->{tree}->isNumber if isFormula($n); 
 	return classMatch($n, 'Real', 'Complex') || $n->{isNumber} || matchNumber($n); 
 } 

and add isNumber to the blessed Fraction objects in the new() and make() methods. This corresponds to one way that classMatch() allows an object to mark itself as a specific class (e.g., if $mo has an isReal property set to true, then classMatch($mo, 'Real') will be true). That is an easy change.

Another would be to look for an isNumber() method on the MathObject itself, as is done in classMatch() so that a MathObject could replace the Value::isNumber() call with its own method, as in

sub isNumber { 
 	my $n = shift; 
 	return $n->{tree}->isNumber if isFormula($n);
 	return $n->isNumber if Value->subclassed($n, "isNumber");
 	return classMatch($n, 'Real', 'Complex') || matchNumber($n); 
 } 

and then add

 	sub isNumber { 1 }

to the context::Fraction::Value::Fraction package.

Finally, and perhaps the best option, is to test the MathObject's type:

sub isNumber { 
 	my $n = shift; 
 	return $n->{tree}->isNumber if isFormula($n);
 	return  (isValue($n) && ($n->type eq 'Number' || classMatch($n, 'Real', 'Complex'))) || matchNumber($n); 
 } 

The classMatch() call here is probably now redundant, but I'm a little hesitant to remove it, as it is a complicated test, but checking that the type is Number should be enough, here. Since Fractions already have type Number, no change would be needed to the fraction context with this change.

I'm not sure which is the best approach, but I'm leaning toward the third one. Discussion welcome.

I was never happy about adding fractions to the signedNumber pattern, and since any of the changes above would no longer rely on the matchNumber() part to have Value::isNumber() be true for a Fraction object, it would mean I could drop the changes to signedNumber.

On a related note, I noted that if I add the studentsMustReduceFractions flag, and enter (10/4, -1), that is still counted correct.

The check for reducing fractions is done in the cmp_postprocess() method for the Fraction object, since it only applies to student answers (not Compute() in general). That means that it doesn't run for Point objects, and so you won't get this message in that case. It would be possible to move the check to when the Fraction object is created, but you would not be able to tell if this were a student answer or not, so would prevent using unreduced fractions when constructing the correct answer, for example. It might be possible for the cmp() method to set a context flag indicating that a student answer is being computed when the student_value is created and use that to help trigger the message, but that seems a bit fragile.

[drgrice1]

I'm not sure which is the best approach, but I'm leaning toward the third one. Discussion welcome.

If you are leaning toward the third approach, then go with that. I agree that that approach seems nicest.

The check for reducing fractions is done in the cmp_postprocess() ...

This is not critical. I can still use a custom grader to check simplification using the isReduced method on the coordinates extracted from the point. I would do so anyway to give partial credit typically. I just thought it would be nice for those that like the typical all or none grader to have this work.

[dpvc]

If you are leaning toward the third approach, then go with that. I agree that that approach seems nicest.

OK, I've made that change. I think this is ready to go from my end, unless anyone else has things they need fixed.

[drgrice1]

Everything seems to be working in my tests. Thanks for your work on this.

The base branch was changed.