Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MathObject version of the new Boolean context #1035

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

drdrew42
Copy link
Sponsor Member

@drdrew42 drdrew42 commented Mar 17, 2024

Refer to #1033 if you really want to see how this sausage got made. Many thanks to @dpvc, as always.

Here it is! A boolean context for MathObjects.

Features

  • parsing and comparison of boolean expressions
  • recognition of equivalent boolean expressions
  • customizable string and TeX for T/F constants and boolean operators
  • unicode alternatives (except for T/F -- see below) for operators
  • reduction rules for 'and' and 'or' with constants, as well as double negation
  • even perlFunctions work!
  • global T/F as $context::Boolean::T and $context::Boolean::F

Unresolved Issues

  • the global T/F can fall out of alignment with the context (e.g. if an author sets T to have string 'true') -- do these globals need to be updated with inContext each time they appear? (in eval, even in perl)
  • Contexts seem unwilling to accept unicode alternatives for constants
    • uncomment the alternatives for T/F and see what happens
  • Parser::Number::eval seems to be the only (?) evaluator that doesn't return a MathObject
    • creates some minor hack-ery in _reduce methods here
  • substitution of variables does not work with perl (hence perlFunction also)
    • no one has noticed for 20-ish years, so clearly not that serious
  • still unclear why this Value::Real subclass doesn't inherit cmp_defaults more details -- see item 1

Future Steps

  • updates to Parser::Item::isNeg link
  • shift responsibility for new variable types from the global hash in Value over to the context link
    • this should also update the context precedence for newly added types
  • update Parser::Variable::perl to fix the substitution of variables issue link
  • rethink the structure of cmp_defaults link -- item 1

Feedback Requests

  • what should the precedence be for the operators? Right now, I have set all operators to the same precedence with the exception of 'not'. Should this instead prioritize 'and' over 'or' (akin to + over *) -- but then where does 'xor' fit?
  • do we want to see /\ and \/ added as alternatives for 'and' and 'or'?
    • will this affect/interfere with the redefinition of '/' for error message purposes? link
  • should our context constants be flagged with isConstant => 1? link -- see item 4
  • should Parser::Number::eval return a MathObject (in alignment with other classes' eval methods)?
  • should the new Boolean BOP/UOPs update the context of the global $context::Boolean::T (or F) when evaling?

@Alex-Jordan
Copy link
Contributor

Alex-Jordan commented Mar 17, 2024

what should the precedence be for the operators?

I'd definitely vote for and to have higher precedence than or. (Or is it "lower"? Whatever makes a or b and c equal to a or (b and c).)

Perl puts & at the top, then |, then ^ (xor), then the negation operator lower than all of them, according to: https://www.tutorialspoint.com/perl/perl_operators.htm. So maybe that is an established order of operations to follow.

@Alex-Jordan
Copy link
Contributor

do we want to see /\ and / added as alternatives for 'and' and 'or'?

I wouldn't be opposed, but also not inclined to promote that. If I were picking something other than the words, I'd pick V and A, but not sure they are necessary. As we see with the U operator in other contexts, having a letter be an operator can get in the way of using that letter as a variable. Also if any of these are used, not sure if an alternative or an alias is best. I think maybe alias.

Are the operators case insensitive? If not, it might be good to make AND, XOR, etc be aliases.

@dpvc
Copy link
Member

dpvc commented Mar 17, 2024

Perl puts & at the top, then |, then ^ (xor), then the negation operator lower than all of them

Note that the &, |, and ^ are bitwise operators, not logical operators, and these do have precedence above the logical operators because you want things like $FLAGS & 0x10 && $FLAGS & 0x1000 to be ($FLAGS & 0x10) && ($FLAGS & 0x1000) (though this is better as $FLAGS & 0x1010, but you get the idea). That is, the bitwise operators act like the binary operators (like plus), and are performed before logical operators.

The logical operators are &&, ||, not, and, or, and xor. It turns out that && and and have different precedences in perl, as do || and or. I presume this is so that you can use not $a && $b to be not ($a && $b) (some people's preferences) while not $a and $b is (not $a) and $b (other people's preference).

I'd recommend (from high to low) not, and, or with xor the same as or (some want it between not and and, however).

If there are different preferences, you might want to have $context->setPreferences(x) where x is 'standard', 'equal', etc. for whatever preference settings you want.

@drgrice1
Copy link
Sponsor Member

Note that you should not mix high and low precedence operators such as not $a && $b. See https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitMixedBooleanOperators.

@drgrice1
Copy link
Sponsor Member

Note the usual rule of thumb for perl is to only use the low precedence operators (or, and, not) for control flow. Not for logic operations in conditionals. So I don't think those should be considered in this discussion. Instead look at &&, ||, and !.

@dpvc
Copy link
Member

dpvc commented Mar 18, 2024

I don't think those should be considered in this discussion. Instead look at &&, ||, and !.

It doesn't really matter which set you are looking at, as the relative precedences within the sets are the same, which is all that we are after here. I used and, or, and not because they also included xor, whereas there is not a ^^ xor operator (presumably because != works for that?). Since this context includes an xor, I wanted to be able to include that.

My example that mixed them above was an attempt to understand why there are two sets in the first place with different precedences (still a mystery to me), and was probably not worth including, except that I wanted to point out that there are differing opinions on what the meaning of ~ a b in Boolean Algebra (the mathematical topic, not a programming language) should be: some want ~(a b) and others (~a) b. This is similar to the issue of whether sin 2x is sin(2x) (the mathematical convention) or sin(2) x (the WeBWorK convention).

Of course, mathematical conventions can't always be encoded in a precedence-based parser, but how the precedences are set here will determine which interpretation of ~ a b is to be used.

@drdrew42
Copy link
Sponsor Member Author

I like the suggestion of a helper method for authors to choose between precedence settings. It seems inevitable that there will be authors on either side. I'm thinking something along the lines of setPrecedence(x) with x being either equal (or = xor = and = not) or oxan (or < xor < and < not).

@dpvc
Copy link
Member

dpvc commented Mar 18, 2024

the global T/F can fall out of alignment with the context (e.g. if an author sets T to have string 'true') -- do these globals need to be updated with inContext each time they appear?

Because each MathObject contains a pointer to the context in which it is created, and because Context("Boolean") creates a copy of the source context, if someone does

Context("Boolean");
Context()->constants->set(T => {string => "True"});

the constant is changed in the copied context, while the original template context is unchanged. But context::Boolean::T points to the context in effect then it was created (probably just the default Numeric context), and so doesn't have access to the modified T constant. Even if one creates context::Boolean::T via context::Boolean::Boolean->new($context, 1), that will still point to the template Boolean context, not the copy where the problem author changed the T string.

It was probably a bad suggestion on my part to create a global T and F, and instead, it would be better to have these be context-specific, so that the copied context has T and F tied to the correct context. This can be done by subclassing Parser::Context and overriding the copy() method. You are going to want to do this anyway in order to add the setPrecedence() method, so why not take advantage of it for this?

The first step is to define the subclass

sub copy {
 	my $self = shift->SUPER::copy(@_);
	## update the T and F constants to refer to this context
	$self->constants->set(
		T => {value => context::Boolean::Boolean->new($self, 1)},
		F => {value => context::Boolean::Boolean->new($self, 0)}
	);
	return $self;
}

## Access to the constant T and F values
sub F { shift->constants->get('F')->{value} }
sub T { shift->constants->get('T')->{value} }

This copy() be called when Context("Boolean") is performed, since that makes a copy of the named context, and it updates the T and F constants to have value that point to the new context. The F and T methods give easy access to those new values (rather than using global ones), so Context()->F and Context()->T get you these.

You could also add

## top-level access to context-specific T and T
sub T {
	my $context = main::Context();
	Value::Error("Context must be a Boolean context") unless $context->can('T');
	return $context->T;
}

sub F {
	my $context = main::Context();
	Value::Error("Context must be a Boolean context") unless $context->can('F');
	return $context->F;
      }

to the context::Boolean package (after the Init() function) so that you can use context::Boolean->T and context::Boolean->F in place of $context::Boolean::T and $context::Boolean::F, provided the current context is a Boolean one.

Then at the end of the Init() function, after adding the constants, add

	## add our methods to this context
	bless $context, 'context::Boolean::Context';

to make your new context be your subclassed one.

If you make these changes, then your createRandomPoints() method can use

	my $T = $context->T;
	my $F = $context->F;

to get the $T and $F, and your perl() methods would use

	return "($result ? context::Boolean->T : context::Boolean->F)";

while context::Boolean::Boolean::perl would use

	return $self->value ? 'context::Boolean->T' : 'context::Boolean->F';

In the same vein, the eval function for or could work as

sub _eval {
  my ($self, $l, $r) = @_;
  return ($l->value || $r->value ? $self->context->T : $self->context->F);
}

with similar changes to the other ones.

Finally, the original constants themselves would be added in Init() using

	## Define constants for 'True' and 'False'
        $context->constants->{namePattern} = qr/(?:\w|[\x{22A4}\x{22A5}])+/;
	$context->constants->are(
		T => {
			value => context::Boolean::Boolean->new($context, 1),
			string => 'T',
			TeX => '\top',
			perl => 'context::Boolean->T',
			alternatives => ["\x{22A4}"]
		},
		F => {
			value => context::Boolean::Boolean->new($context, 0),
			string => 'F',
			TeX => '\bot',
			perl => 'context::Boolean->F',
			alternatives => ["\x{22A5}"]
		},
		'True'  => { alias => 'T' },
		'False' => { alias => 'F' },
	);

Contexts seem unwilling to accept unicode alternatives for constants

Note the line at the top of the last perl code block above that changes the constants' namePattern to allow the alternative names to be allowed. (The original pattern only allowed an alpha-numeric name.) While this pattern only allows the two alternatives given you could make the name pattern more generic using qr/.+/, to allow any (non-empty) name.

I had included this in a comment earlier, but it would be easy to miss.


Parser::Number::eval seems to be the only (?) evaluator that doesn't return a MathObject

Yeas, that probably wants to be changed. In the meantime, you can subclass Parser::Number to get better results from eval and perl:

## Subclass Parser::Number to return the constant T or F
package context::Boolean::Number;
our @ISA = ('Parser::Number');

sub eval {
  my $self = shift;
  return $self->context->constants->get(('F', 'T')[$self->{value}])->{value};
}

sub perl {
  my $self = shift;
  return $self->context->constants->get(('F', 'T')[$self->{value}])->{perl};
}

and add

        $context->{parser}{Number}      = 'context::Boolean::Number';

at the top of the Init() function. That will allow you to simplify the reduction functions. E.g., for or:

sub _reduce {
	my $self    = shift;
	my $reduce  = $self->context->{reduction};
	my $l       = $self->{lop};
	my $r       = $self->{rop};

	return $self unless ($l->{isConstant} || $r->{isConstant});

	if ($l->{isConstant}) {
		return $l->eval->value ? ($reduce->{'x||1'} ? $l : $self) : ($reduce->{'x||0'} ? $r : $self);
	} else {
		return $r->eval->value ? ($reduce->{'x||1'} ? $r : $self) : ($reduce->{'x||0'} ? $l : $self);
	}
}

(note also the use of $self->context rather than $self->{equation}{context}, which is just a simpler method of getting the same thing.)


I'm thinking something along the lines of setPrecedence(x) with x being either equal (or = xor = and = not) or oxan (or < xor < and < not).

To do that, you can add

## Easy setting of precedence to different types
sub setPrecedence {
	my ($self, $order) = @_;
	if ($order eq 'equal') {
		$self->operators->set(
			or  => {precedence => 3},
			xor => {precedence => 3},
			and => {precedence => 3},
			not => {precedence => 3},
		);
	} elsif ($order eq 'oxan') {
		$self->operators->set(
			or  => {precedence => 1},
			xor => {precedence => 2},
			and => {precedence => 3},
			not => {precedence => 6},
		);
	} else {
		Value::Error("Unknown precedence class '%s'", $order);
	}
}

to the context::Boolean::Context package.


I've added a copy of the context file with the modifications I mentioned (I think I included everything I changed).

contextBoolean.pl.zip

@dpvc
Copy link
Member

dpvc commented Mar 18, 2024

do we want to see /\ and \/ added as alternatives for 'and' and 'or'?

  • will this affect/interfere with the redefinition of '/' for error message purposes? link

I suggested it because the TeX output looks like these, and you are already using >< as a multi-character operator, so thought these would be ways ASCII ways to make the symbols the students will see. I don't have strong feelings either way.

I'm not sure what problem you are anticipating about interference with /, which will be unaffected by these definitions. If the input could be treated as \/ or /\ rather than /, then it will be (longer operator names are parsed before shorter ones).


should our context constants be flagged with isConstant => 1

That certainly would be a reasonable work-around for now, until the core constant implementation is fixed to include that.


should Parser::Number::eval return a MathObject (in alignment with other classes' eval methods)?

I would say yes, but some checking is probably in order to make sure that there are not unwanted side effects. I can't think of any, but there could be. One result will be that computations using the evaluated result will be slower since they go through the MathObject overloads rather than the direct perl computations. Also, comparisons will be done with the fuzzy tolerance parameters, and that might lead to unexpected results in some cases, particularly comparisons like $f->eval() > 0.


should the new Boolean BOP/UOPs update the context of the global $context::Boolean::T (or F) when evaling?

No, I would go with the context-specific T and F values, as indicated in my previous message.

Copy link
Sponsor Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that is missing from this at this point is POD. But perhaps you are waiting until the code is ready to work on that?

Perhaps 'vee' should be added as an alternate for 'or', 'wedge' an alternate for 'and', and 'oplus' an alternate for 'xor'. Those are the texts that MathQuill outputs when those symbols are entered. So if buttons are added to the toolbar for these logic symbols, then those answers would be considered correct. The alternates could also only be added when MathQuill is enabled. Perhaps the text output of MathQuill could be changed, but I am not sure that is a good idea since those symbols have other meanings in different mathematical contexts.

I noticed something unexpected in testing this. Consider the following minimal example:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'contextBoolean.pl');
Context('Boolean');
$ans = Formula('not (p or q)');
BEGIN_PGML
Enter [`[$ans]`]: [_]{$ans}
END_PGML
ENDDOCUMENT();

In that example $ans is displayed as ~p∨q, yet if you enter not p or q it is counted as incorrect. It seems that the parentheses should not be removed when this is displayed. This may be related to the precedence or not and or.

Comment on lines 23 to 26
## Define our logic operators
# (for now...)
# all binary operators have the same precedence and process left-to-right
# any parens to the right must be preserved with consecutive binary ops
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use this kind of comment. That is just use a single # for comments, and don't use the reverse indented style. With syntax highlighting in any modern text editor, there is no need for additional #s to offset comments. It completely baffles me where this reverse indented comment style comes from.

Preferably comments would be full sentences with punctuation and capitalization.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yeah, this will get cleaned up as we decided on precedence options -- and related to your other comment, something with the parens config is off...

Also, regarding POD, you're right, and I was waiting for the dust to settle on some of the pending issues.

@drdrew42
Copy link
Sponsor Member Author

drdrew42 commented Apr 3, 2024

I looked into the parens issue with 'not' that @drgrice1 outlined above, and it seems that UOPs do not pass along a 'showParens' argument. As a result, the child BOP ('or' in the example above) does not add parens when the precedences are the same (because $showParens is undef, and all our BOPs have 'left' associativity).

pg/lib/Parser/BOP.pm

Lines 336 to 343 in 5360cb6

my $addparens = defined($precedence)
&& (
$showparens eq 'all'
|| (($showparens eq 'extra' || $bop->{fullparens}) && $extraParens > 1)
|| $precedence > $bop->{precedence}
|| ($precedence == $bop->{precedence}
&& ($bop->{associativity} eq 'right' || $showparens eq 'same'))
);

Just to confirm with @dpvc, the right move here is to add a showParens to the UOP def hash (since left or right parens don't make sense with a unary operator). Then that parameter should be added as an argument to the recursive string call in the UOP string method.

@dpvc
Copy link
Member

dpvc commented Apr 4, 2024

It looks like I never considered the possibility of a unary operator having the same precedence as binary operators that they may apply to.

the right move here is to add a showParens to the UOP def hash (since left or right parens don't make sense with a unary operator). Then that parameter should be added as an argument to the recursive string call in the UOP string method.

I think this may actually be overkill, as I think that if the precedences are the same, you should always use parentheses, so there is no need for a showParens property in the def hash. I don't see a case where altering that would make sense. (but if you do want to add that, then the default should be same rather than an empty string). So I would just add "same" as a second argument to both string() calls in the UOP string() method.

You will also need to do something similar to the TeX method, but it is a bit more complicated there, as there is a nofractions value that has to be dealt with. (It looks like the "" should be "same" in the definition of $fracparens.)

For now, you could add

sub string {
  my ($self, $precedence, $showparens, $position, $outerRight) = @_;
  $showparens = "same" if !($position // '') && !($showparens // '');
  return $self->SUPER::string($precedence, $showparens, $position, $outerRight);
}

sub TeX {
  my ($self, $precedence, $showparens, $position, $outerRight) = @_;
  $showparens = "same" if !($position // '') && !($showparens // '');
  return $self->SUPER::TeX($precedence, $showparens, $position, $outerRight);
}

to the context::Boolean::BOP package to get the desired parentheses until the Parser::UOP is updated.

Comment on lines 72 to 73
class => 3,
precedence => 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to change the class to 3 and leave the precedence as 1? I think you may have meant to change the precedence so that implied multiplication is the same precedence as everything else.

That also points out that I forgot to change the precedence of ' ' in the setPrecedence() method, so you should probably add that in.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, great catch here.

@drdrew42
Copy link
Sponsor Member Author

drdrew42 commented Apr 4, 2024

So I would just add "same" as a second argument to both string() calls in the UOP string() method.

Yeah, that absolutely makes sense. No need to make that configurable.

Copy link
Sponsor Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ready now.

Copy link
Sponsor Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good at this point. Let us know when you are ready for this to be merged (I think you said you have something else you wanted to do yet).

@drdrew42 drdrew42 merged commit 7806ca7 into openwebwork:develop Jul 2, 2024
3 checks passed
drgrice1 pushed a commit that referenced this pull request Jul 2, 2024
MathObject version of the new Boolean context
@drgrice1
Copy link
Sponsor Member

drgrice1 commented Jul 2, 2024

This was actually merged into develop. I added to the PG-2.19 branch as well now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants