openwebwork / pg

Problem rendering engine for WeBWorK
http://webwork.maa.org/wiki/Category:Authors
Other
45 stars 76 forks source link

Context("LimitedNumeric") returns Unexpected character when using parentheses #568

Open somiaj opened 3 years ago

somiaj commented 3 years ago

I was writing a problem in which students need to compute P(n,k) or C(n,k) in a simplified form using Context("LimitedNumeric"). Here is the .pg file.

DOCUMENT();
loadMacros(
  "PGstandard.pl",
  "MathObjects.pl",
  "PGauxiliaryFunctions.pl",
  "contextIntegerFunctions.pl",
  "contextLimitedNumeric.pl",
  "unionLists.pl",
  "PGcourse.pl"
);

TEXT(beginproblem());

Context("IntegerFunctions");
$n1 = random(7,12,1);
$k1 = random(3,$n1-3,1);
$q[0] = "P($n1,$k1)";
$a[0] = P($n1,$k1);
$q[1] = "C($n1,$k1)";
$a[1] = C($n1,$k1);

Context("LimitedNumeric");
Context()->flags->set(
  tolerance => 0.0001,
  tolType => "absolute",
);
$ans[0] = Compute($a[0]);
$ans[1] = Compute($a[1]);

Context()->texStrings;
BEGIN_TEXT
Compute each of the following. Answers must be fully computed
as a single integer (no formulas/operations allowed).

\{ BeginList("OL", type=>"a") \}
$ITEM
\($q[0] = \) \{ ans_rule(5) \}

$ITEMSEP
$ITEM
\($q[1] = \) \{ ans_rule(5) \}

\{ EndList("OL") \}

END_TEXT
Context()->normalStrings;

ANS($ans[0]->cmp);
ANS($ans[1]->cmp);

ENDDOCUMENT();

In testing this with MathQuill I tried to enter in an answer n!/(n-k)! such as 8!/5! to see what was returned and the message was Unexpected character '(' as opposed to Can't use '!' in this context since MathQuill adds extra parenthesis. I think this context should probably allow parenthesis and thus accept answers like (336) so a more appropriate message is returned when students enter in something like 8!/5! since it might confuse them since they didn't actually enter in any parenthesis.

drgrice1 commented 3 years ago

I agree. This is not the only context with this problem. Parentheses have been incorrectly dealt with in many of the contexts. They are just blindly disabled instead of dealing with them properly. contextScientificNotation.pl does the same thing.

somiaj commented 3 years ago

I think I found the offending line:

https://github.com/openwebwork/pg/blob/93f121c5aa9eb407415e56462c4751e72aba39cc/lib/Parser/Legacy/LimitedNumeric.pm#L90

To get just parenthesis back (which is done in LimitedNumeric-Fraction and macros/contextFraction.pl), that line could be replaced with $context->undefine('|','{','[');, though unsure if { and [ should be accepted in these contexts too.

Grepping the source, I only found LimitedNumeric undefined all the parens this way, while only LimitedNumeric-Fraction and macros/contextFraction.pl used this to undefined |, {, [.

It appears that contextScientificNotation.pl uses $context->parens->clear; to remove the parenthesis. Here is the list of other contexts that clear all parens this way.

macros/contextScientificNotation.pl:  $context->parens->clear;
macros/contextOrdering.pl:  $context->parens->clear();
macros/parserPopUp.pl:  $context->parens->clear();
macros/contextString.pl:  $context->parens->clear();
macros/parserWordCompletion.pl:  $context->parens->clear();
macros/contextReaction.pl:  $context->parens->clear();
macros/contextArbitraryString.pl:  $context->parens->clear();
dpvc commented 3 years ago

Parentheses have been incorrectly dealt with in many of the contexts. They are just blindly disabled instead of dealing with them properly.

I couldn't disagree with you more. Whether this is "incorrect" should be based on what the designer intended, and while that may not meet your requirements, that doesn't mean it is incorrect; it means you need a different tool that better matches your requirement. It's like trying to eat soup with a fork and complaining that the fork is not designed correctly, when really you are just using the wrong tool.

Before the MathObjects library was available, answers were checked via functions like std_num_cmp() and strict_num_cmp(), and so on. After MathObject was added, these functions were replaced by MathObject-based equivalents. The LimitedNumeric context was designed to replace the strict_num_cmp() family of functions (you will see that the definition of LimitedNumeric is actually in pg/lib/Parser/Legacy). Since the original strict functions did not allow parentheses, LimitedNumeric didn't either. So LimitedNumeric is doing exactly what it is intended to do, and that is not "incorrect" behavior.

Similarly, the ScientificNotation context was built in 2006 at an AIM conference where Robin Cruz requested this functionality. My recollection is that she wanted this for a high-school setting where the syntax was very specific and relatively rigid. Something like (2.123)x(10^(-5)) would not have met those requirements, and so the lack of parentheses was a design decision, not a mistake. Equivalent presentations were not what she was looking for.

Now that you are using MathQuill, which inserts unneeded parentheses, you have different requirements. That doesn't mean the existing contexts are incorrect, it means they no longer match your new requirements. When tools like MathQuill where being added to MathJax, I pointed out that they would not play nice with some contexts, as they have no information about what the contexts allow or how they interpret the symbols being used. This is one example where that sort of problem exists.

While you can certainly re-enable parentheses in these contexts, I would object to that in both these cases. If I use LimitedNumeric, I would not want to allow (-((123))) in place of -123 in general, which is what blindly adding the parentheses back in would do. Similarly, for ScientificNotation, I would not want to allow ((1.23)x(10^(-5))), which adding parentheses back in would allow. Some people may want that, but not all. The point of the limited contexts is to enforce specific syntax, not necessarily to accept all equivalent forms.

If you are going to add them back in anyway, it should at least be an option, rather than the default. The problem, of course, is that the problem sets up the Context, and doesn't know whether MathQuill is being used or not, so doesn't know whether to allow the parentheses or not. One possibility might be to have MathQuill answers first be parsed in a general context and stringify the result (so that unneeded parentheses are removed), then pass the result to the original answer checker. There are some caveats to doing this (e.g., one would want reduceConstants => 0 and reduceConstantFunctions => 0, and in order to keep the original precision of the numbers entered by the student, one would need to stringify using the Parser::Number's value_string rather than the numeric value), but most of those could be dealt with. Of course, this could introduce other issues with contexts that are far from the usual numeric context, but MathQuill is not well suited to those to begin with.

Note also that those who would like to have parentheses allowed in these contexts could do that through their own templates/macros/parserCustomization.pl file by adding something like

$main::context{LimitedNumeric} = Parser::Context->getCopy("LimitedNumeric");
$main::context{LimitedNumeric}->parens->redefined("(");

loadMacros("contextScientificNotation.pl");
$main::context{ScientificNotation} = Parser::Context->getCopy("ScientificNotation");
$main::context{ScientificNotation}->parens->redefined("(");

so that when these contexts are called on, you will get the modified version with parentheses allowed. Perhaps something that does this automatically for MathQuill-endabled problems could be arranged.

drgrice1 commented 3 years ago

@dpvc: I apologize. I didn't mean to offend. You are correct in that whether or not has been dealt with correctly depends on the intent of the design. I understand the original intent, although I don't entirely agree with the pedagogy in that intent. What is wrong with answers like (-((123))) or ((1.23)x(10^(-5)))? They are mathematically correct answers. Note MathQuill does not add parentheses like those. I could understand saying they are not entirely simplified.

In any case, a context can know whether MathQuill is used or not, we just need to set it up. For now $envir{entryAssist} could be checked. If MathQuill is enabled, then that will have the value MathQuill. Although, that is not entirely reliable. If MathQuill is enabled for the course, yet the user disables it in their own settings, that will still have the value of MathQuill. So we just need to add a better flag to pass in to PG that tells it that MathQuill is enabled.

somiaj commented 3 years ago

Parentheses have been incorrectly dealt with in many of the contexts. They are just blindly disabled instead of dealing with them properly.

I couldn't disagree with you more. Whether this is "incorrect" should be based on what the designer intended, and while that may not meet your requirements, that doesn't mean it is incorrect

Thanks for the clarification and history. Though I have one point of confusion, LimitedNumeric-Fraction only uses $context->undefine('|','{','['); (thus keeping parenthesis around). Could this because of a similar issue we now have with MathQuill that fractions sometimes have extra parenthesis and this was a way to better deal with that case? Since some of the issues you mention would affect LimitedNumeric-Fraction but not LimitedNumeric.

dpvc commented 3 years ago

@drgrice1

I understand the original intent, although I don't entirely agree with the pedagogy in that intent.

People differ in their pedagogical focus, and that's fine. The point is, some people do have a more strict requirement than others. That doesn't make it wrong, even when we don't agree with it.

What is wrong with answers like (-((123))) or ((1.23)x(10^(-5)))? They are mathematically correct answers.

If the correct answer is -123, then -100-23 is also mathematically correct, and so is -sqrt(123^2), but these will not be marked correct in the LimitedNumeric context. The whole point of the limited contexts is that not all mathematically equivalent answers are considered correct. Teachers may differ where to draw that line, but mathematical equivalency is not a sufficient reason to be marked correct, since some mathematically equivalent answers are being excluded by design. Contexts like the limited contexts and the bizzaro contexts are all about enforcing particular formats.

@somiaj

LimitedNumeric-Fraction only uses $context->undefine('|','{','['); (thus keeping parenthesis around). Could this because of a similar issue we now have with MathQuill that fractions sometimes have extra parenthesis and this was a way to better deal with that case?

I'm afraid I don't remember the details (this would have been back around 2005). I would need to go back to the original fraction answer checkers that the LimitedNumeric-Fraction was supposed to replace. It may have been to allow -(1/2), but you are right, it allows more, and I probably should have limited that further. It was not for extra parentheses like MathQuill adds, however. That is, I suspect that I was not trying to allow (1)/(2), which of course it does allow. But again, I would need to go back to the original fraction answer checker to see what I was trying to mimic.

drgrice1 commented 3 years ago

I didn't say that is was wrong. I just said that I don't agree with it. I also said that I could understand not considering answers like (-((123))) being simplified. Although, in my opinion there is a considerable difference between things like that, and things like -100-23 and -sqrt(123^2) in terms of simplification concepts that I would consider more important. Students are rarely (if ever) going to add extra parentheses. The real problem is not adding the necessary parentheses.

dpvc commented 3 years ago

Well, now apparently I've offended. If so, I'm sorry.

drgrice1 commented 3 years ago

No offense taken.

I think we just need figure out a way to make PG properly determine if MathQuill is enabled or not, and in that case allow parentheses. Either that or we need to completely rewrite MathQuill to be a mathematical parser as well as a latex parser. That will take quite a bit of work.

somiaj commented 3 years ago

Since it is fairly easy to change the behavior in a problem by using either $context->parens->undefine('(') or $context->parens->redefine('(') (depending on if the default is to allow parentheses or not), it may not be worth the effort to have problems use different defaults depending on which input method is being used (though it could be this feature has uses elsewhere). To me it is a matter of which default is more desired.

I personally think having a bit more lenient defaults by allowing parenthesis for these contexts is useful for newer problem authors and users (such as myself). Students seem to like MathQuill, so I suspect it is going to be used more often as the default input and I don't see students generally abusing allowing extra parenthesis and entering in answers which would be valid but not wanted by instructors in these contexts. Then those who want to be more restrictive can both undefine the parenthesis and use the traditional input method.

Though, I'm also okay with defaults being more restrictive, but it will be something new users will have to be aware of if they use these contexts with MathQuill (Until I started digging into this, I would have not known how easy it was to change the behavior).

Alex-Jordan commented 3 years ago

My thoughts:

it may not be worth the effort to have problems use different defaults depending on which input method is being used

This is important enough to work on, when time allows. With centralized problem libraries used by multiple instructors, and the need to carry course setup over from one term to the next, it is not a solution to ask instructors to customize problem code with a context modification like that. Especially if it comes down to the instructor changes their mind about using (or not using) MathQuill. Toggling that should not require visiting problems and customizing the problem code.

It will just have to wait until a future release. It would be nice if there were funding to pay someone to do these things.

I personally think having a bit more lenient defaults

It could be damaging to change defaults like these for the entire WW-using community. This one in particular has been in place for so long and there certainly are cases where the current default behavior is expected, and something more lenient would be unwanted. We should be very careful about changing defaults out from under the entire user base.

But, there is nothing wrong with customizing one's own server. I do that frequently. It just helps to have a plan for how you manage such customizations while still pulling improvements from GitHub.