Introducing a "Boolean" context

[drdrew42]

I know perl and booleans don't mix. That didn't stop me from trying 😂

Most of this is pretty straightforward, adding some new binary ops for 'and' and 'or', and a unary for 'not'. I annexed addition and multiplication to save some effort -- but I could be convinced that this isn't the right move.

However, I didn't want the stringified versions of these operations to remain '+', '*', and '-', which meant pushing more tokens onto the operators. This is necessary in order to handle something like $f=Formula("p+q"); followed by $g=Formula("-$f");

Formula creation seeds the Formula object with {test_points} and {test_values} for all combos of the variables in the context. This enables the comparison of boolean formulas for equivalence.

perlFormula is also supported.

I'm not sure if anything is necessary for _check or wanted for _reduce.

Keeping this as a draft for now, let's get some feedback!

[dpvc]

This is a nice addition, but there are some issues that I have commented on that you might want to think about.

[drdrew42]

Thanks @Alex-Jordan and @dpvc for the feedback, I've incorporated most of the recommended changes.

By adding a Value::Type(Boolean) to the end of the _check methods in BOP and UOP, all of the types in the tree are reporting 'Boolean' as expected. (Using p and q in a Formula, for that)

I'm still struggling with the T/F constants, however. They keep ending up as Reals when Compute()ed. I suspect it has something to do with the value in their definition.

I also now think that L#12 should be $context->{parser}{Number}, pointing at my Parser::Number sub-class, but something still isn't right. I can overwrite typeRef safely (to return Value::Type('Boolean')), but that still isn't enough.

I've got to break for today's responsibilities -- happy to receive any suggestions before I'm back at it again tomorrow.

[drdrew42]

Okay, this is coming along... there are some lingering questions left in the code (and I'm still not clear on replacing Parser::Number -- is this the reason 1 and 0 aren't parseable in strings via Compute/Formula?)

I've added 1 and 0 as constants to the context -- which may be the 'right' thing to do anyhow.

I noted that Constants don't have a type method (having just the key-value and no accessor). This makes things clunky for BOP and UOP _check.

Another oddity I encountered is that getValueType has backwards arguments for subclasses compared to Parser::Value::getValueType.

[dpvc]

You have made a number of important improvements, here, but have also started to work too hard in some places and are starting to go in the wring direction. I've made a lot of comments, but do be discouraged. I think you are almost there.

I tried to explain my recommendations, but if anything isn't clear, let me know. I will write some more about the questions you asked (concerning {parser}{Formula} and {value}{Formula}, and some of the history about why things are as they are. I'm hoping that will help clarify the how some of this is set up.

[dpvc]

Since $_[1] and $_[2] will both be MathObjects, if you use them with ||, you will always get 1, as they are truthy as object references. So you need

sub _eval { return $_[1]->value || $_[2]->value ? 1 : 0 }

here and in the other _eval methods below.

[drdrew42]

Yep, I figured this one out 👍

[drdrew42]

Although it is also necessary to ensure that the result is a MathObject here as well, since returning a raw perl value will not allow ops higher in the tree to call ->value...

So I have

sub _eval { return $_[0]->Package('Boolean')->new($_[1]->value || $_[2]->value ? 1 : 0) }

[dpvc]

It is probably better to use

sub _eval { return $_[1]->value || $_[2]->value ? $context::Boolean::T : $context::Boolean::F) }

so you don't have to keep creating new objects (the constructors do a lot of argument checking). Otherwise, when you know the arguments are valid, use make rather than new, for efficiency.

[drdrew42]

Makes sense to me 🤡

[drdrew42]

@dpvc Thanks for putting me back on track! Things really were starting to get kludge-y there...

Most of what you've proposed makes sense to me, and it feels good that some of your suggestions were things I'd already uncovered myself. I think things are really starting to fall into place as far as my understanding of PG goes. I still have a few things to clear up, and my thanks in advance for your continued patience and detailed feedback!

1. This is likely some perl arcana rather than anything PG-related, but I'll ask it anyhow. context::Boolean::Boolean subclasses Value::Real, but seemingly it does not inherit cmp_defaults from the parent class. ('Method not found' when using a Boolean Formula as the correct answer for an answer rule.) I have found that providing a simple pass-through (sub cmp_defaults {shift->SUPER::cmp_defaults(@_)}) in the child class resolves the issue, but it seems that making this method explicit shouldn't be necessary. After all, isn't this what inheritance is supposed to do automatically?

   The only potential cause that I can find here is that cmp_defaults for Value::Real is actually declared in a separate module (AnswerChecker.pm) -- but then cmp_defaults isn't called without this module being loaded. However, cmp_defaults would be called from 'early' in the module, and the Value::Real::cmp_defaults method isn't declared until 'later' in the module, so maybe that could be relevant? I haven't been able to find any online materials that deal with this type of situation; understandably, as we are pretty deep in the weeds here.

2. With the recommended changes you've proposed, I was still getting different behavior from Formula when my input string includes '1' or '0' instead of using 'T' or 'F'. When using numbers, the eval method method choked because the incoming BOP/UOP operands were raw perl values; and when using T/F, BOP/UOP received blessed objects. (This issue with eval then also caused Compute to choke when parsing any string with 1/0 instead of T/F.)

   Digging into the tree, I saw a difference in that nodes have class Parser::Constant when using T/F strings, and class Parser::Number when using 1/0. These two classes handle eval differently, with Parser::Number directly passing back the value property (I presume this is never going to be a blessed object); while Parser::Constant is deliberate in returning a re-context-ualized MathObject when the value is blessed.

   Three solutions came to mind, but perhaps the simplest is to avoid defining the T/F constants with MathObject values, instead making them perl-valued 1/0. This is easier than overloading Parser::Constant::eval or Parser::Number::eval to make them compatible. With the constants set to have values that are unblessed, the BOP/UOP _eval methods can be greatly simplified (arguments and returned values being strictly perl-valued), and further, the Formula class will bless the final result anyhow (similar to what we saw with perl and perlFunction). I've got this 'working' for now (needs further testing), but I'd feel better with your confirmation about this approach.

3. I'll do some digging on this one, as it wasn't even on my radar -- but you suggested $context->{precedence}{Boolean} = $context->{precedence}{Real}; and I don't know where this comes into play. I presume it relates to deciding on parenthesis during string/tex-ification? This suggested change might actually resolve one of my other questions about parenthesis in string outputs, so I'll hold off on asking about that. I'll just add that I've included rightparens => 'same' in the operators and made them all (with the exception of 'not') the same precedence -- I don't think there's any PEMDAS equivalent for binary operators.

4. The last question deals with some reduction rules that weren't present in the code you reviewed. I responded above to one of your suggestions, and I'm noting it here so that it isn't overlooked. I understand your feedback about context operator tokens '-' and 'u-', and I would like to support 'p -q' as 'p and not q' as you described. Now that I reflect on my earlier response to your comment, I think I can just overload isNeg on UOP::not to accept the '-' token (instead of 'u-'), and all the other operators would fail the check anyhow, so this should suffice. (Actually, I think this means I can make 'not' the root operator token -- in keeping with the other operands -- and make '-' an alias since I would no longer be dependent on Parser::Item::isNeg.)

   However, related to reducing expressions in this context -- I was playing around with reducing expressions such as 'p or T' => 'p' and 'p and F' => 'F'. After incorporating your latest feedback, I got a little stuck. I have only Number types with classes Variable, Number, and Constant appearing in my parser tree. If I'm understanding correctly, this is all to be expected because we're working on the Parser side, and none of our Boolean types are present because they all live on the Value side. (Understanding where the line between Parser and Value is -- that seems to be the whole crux of the matter!)

   Still, I need to distinguish the constant values in order to apply the desired reductions. All three variants will return true for isNumber, so that doesn't help. Parser::Number is assured to have property isConstant set to 1, but Parser::Constant is not by default (though we can add isConstant => 1 to our definition of T/F), and of course Parser::Variable will not have this property at all. I guess my question is why context constants aren't flagged with isConstant by default? Is this more to do with the method being interpreted as 'isClassName' rather than 'hasConstantValue'?

5. Okay, sorry, one last question -- I thought I'd come across some way of comparing parser trees for equivalence, but now I cannot seem to find it again. Currently, in this context, equivalence of Formulas is effectively based on the complete truth table results across all combinations of T/F for the variables. If one wanted to compare more literally, I would assume we might compare the trees (I don't want to get into a bizarroOps situation)... is that feasible?

Again, let me express my gratitude -- not only for your thorough review of my work -- but for having established this framework for us all. 🙏

Updated commit incoming!

[dpvc]

Looks like you are making real progress. Here are some responses to your questions above.

1. This is likely some perl arcana rather than anything PG-related, but I'll ask it anyhow. context::Boolean::Boolean subclasses Value::Real, but seemingly it does not inherit cmp_defaults from the parent class.

This is caused by the Value::Formula::cmp_defaults() function at

pg/lib/Value/AnswerChecker.pm

Line 1846 in 26f77df

&{ $type . 'cmp_defaults' }($self, @_),

which is trying to call the cmp_defaults function for the type of object that the Formula produces. It does this by constructing a direct reference to the class's cmp_defaults() function, and since this isn't using -> notation, there is no inheritance performed. The reason for this is that it is passing the Formula as the $self, so can't use $self-> or Value::Real-> or whatever, as that would get the wrong cmp_defaults() or the wrong $self. The need to pass the correct $self is because some cmp_default() methods actually use the $self to determine the defaults (e.g., the List class).

This function probably needs to be rethought. I think what it probably should be doing is taking an actual value from the function and using its cmp_default rather than trying to figure that out in other ways. (There are a number of weak points in the Parser/Value libraries that come from before they were as extensible as they now are, where they are not as flexible as they should be, and this is one of them.) It's not always easy to get a return value from the formula (since you don't know the domain), but you could perform a an equality check (say $f == $f) which will force the {test_values} to be created, and then take the first of those as a representative output value whose cmp_defaults() could be used.

---

2. I was still getting different behavior from Formula when my input string includes '1' or '0' instead of using 'T' or 'F'.

Can you give an explicit example of this? I'm not able to reproduce this, but I probably don't have the same version of contextBoolean.pl that you do. I have not had any trouble with 1/0 versus T/F in things like Compute("p or 1") or Formula("p or 1").

For example, with the changes that I have suggested (using constant T with value => $T and perl => '$context::Boolean::T', with the perl()method that constructs$resultexplicitly with->valuerather than throughSUPER::perl(), and with the createRandomPoints()that uses$Tand$F), then Compute("p or 1")->perl` produces

context::Boolean::Boolean->new(($p->value || $context::Boolean::T->value ? $context::Boolean::T : $context::Boolean::F))

which is what I would expect. If you mean when you use ->perl(p=>1), then that is the problem with the Parser::Variable::perl() function that you identified, and should be solved by the changes we discussed above.

---

3. you suggested $context->{precedence}{Boolean} = $context->{precedence}{Real}; and I don't know where this comes into play.

The class precedences are used to determine how values are promoted when two MathObjects are used in combination (e.g., when added or multiplied). For example, if you have

$a = Real(1);
$b = Complex(2,3);
$c = $a + $b;

then in the addition, which MathObject's add() method should be called? Both $a and $b have add() methods, so a determination must be made. This is done via the $context->{precedence} hash. The classes of $a and $b are used to look up their precedences, and the highest precedence has its add() function used, and the other value is passed to it. In this case, $b is complex, with higher precedence than $a, which is a real, and so you get Complex::add($b, $a, 1), where the 1 means the arguments have been reversed in order to get the proper $self object. Then Complex::add() calls Value::checkOrderWithPromote($b, $a, 1), which promotes $a to a Complex object, and returns ($b, promoted $a, $b, promoted $a), which is assigned to ($self, $l, $r, $other) so that $l and $r are the left and right operand of the +, both as complex numbers, and $self and $other are the $b and promoted $a, since you can't tell which was which within the $l and $r.

So {precedence} tells which object class gets to handle the operations between different classes. We see that Complex gets to handle operations between Complex and Real values, and similarly, Vector handles actions between Points and Vectors, or Reals and Vectors, while Matrix handles actions between Matrix and Vectors, or Matrix and Point, or Matrix and Real, etc. Union will handle actions between Unions and Intervals or Unions and Sets, and so on.

In the Boolean context, the only class other than Real (i.e., Boolean) would be List (if you added back the , operator), so setting the precedence properly would allow actions between Lists and Booleans, though there isn't much that can happen there. It's just good to make sure everything is set up in case the context is copied and extended by someone else.

---

4. I think I can just overload isNeg on UOP::not to accept the - token (instead of u-), and all the other operators would fail the check anyhow, so this should suffice.

Yes, as I mentioned, overriding isNeg on UOP::not` is the way to go, but you can just use

sub isNeg {1}

as you know that when this function runs, you know the class is UOP and the operator tied to you context::Boolean::not, so it is a not operator by definition. (Unless someone has subclassed not and made it something other than not, in which case it is up to them to override isNeg as well). So no need to make any of the checks in your current isNeg.

I think this means I can make 'not' the root operator token

Yes, that is one of the nice outcomes from this.

Understanding where the line between Parser and Value is -- that seems to be the whole crux of the matter!

Yes, that is crucial, and I will write more about it later. The Value library is the computational end, while the Parser is the structural end. So you can think of the Parser as the syntactic side and the Value library as the semantic side.

I need to distinguish the constant values in order to apply the desired reductions

If $l is the left operand and if $l->class eq 'Constant', then $l->{def}{value}->value will be 1 or 0. Is that what you re after?

Parser::Number is assured to have property isConstant set to 1, but Parser::Constant is not by default.

True. "Constant" is potentially a misnomer, as it is possible to have Formula-valued constants. On the other hand, the Constant class should do better about handling its values. First, the Parser::Context::Constant::create() function should probably force the value to a MathObject rather than allowing it to be a perl value. Then Parser::Constant::new() should set {isConstant} based on $value->{isConstant}.

---

5. I thought I'd come across some way of comparing parser trees for equivalence, but now I cannot seem to find it again

There isn't anything built-in for that. Perhaps someone did something like that in a separate macro package, but there isn't one in the Parser. I'm not sure what your use case is, but I suspect it would have to take into account an operator's commutativity at least, for which we don't have any data (but you could add that), and potentially associativity, if you want (p and q) and r to be accepted as the same as p and (q and r). It might be possible to use reduction rules to normalize the expression first and then compare the trees.

[dpvc]

I've included rightparens => 'same' in the operators and made them all (with the exception of 'not') the same precedence -- I don't think there's any PEMDAS equivalent for binary operators.

Well, its true that in Boolean algebra, there are different preferences for how to handle this; in particular some people prefer that EVERY operation be parenthesized, while others prefer to use the order or precedence. For most programming languages, that has "or" lower than "and" lower than "not". I've seen some that put "xor" between "and" and "not".

With this, p or -p q would be p or ((-p) and q), while with equal precedences, it would be (p or (-q)) and q. I would expect it to be the former not the latter myself. Of course, someone could always change the precedences to suit their desires, but I think it makes sense to use the precedences that seem the most commonly used ones ("or" after "and"). That would seem particular useful if you are using the notation p + -q*p, where the usual precedences of + and * would match that of the or and and.

[drdrew42]

Closed in favor of #1035

[drdrew42]

For item 1, I'll push this onto the stack of future issues.

For item 2, refer to #1035 code for _reduce and the workaround for eval with regards to Parser::Constant and Parser::Number (related, I think, to item 4)

Item 3, perfect, thank you. I started down the road of looking into what happens with perl addition/multiplication of these Boolean Formulas and did not continue. With this in hand, I'll explore this rabbit-hole.

Item 4, I need to identify more than ->class eq 'Constant' as I want to apply reductions to Parser::Number as well, since those are also 'constant' (and would reduce e.g. 'p or 1'). I ended up resolving this issue by looking for the isConstant flag to filter out any Parser::Variable operands, and then using string to determine 'values' that will resolve the reduction.

Item 5, okay -- maybe it was another context (or maybe I dreamed it 😆), you're right that there are probably too many things to control for.

Bonus I'll add these precedence options to the request for feedback on #1035