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

Make function parenPrecedence greater than unary negative, postive #487

Merged
merged 1 commit into from
Mar 13, 2021

Conversation

Alex-Jordan
Copy link
Contributor

This may be controversial, so consider this PR as up for discussion.

In the forums, there is this thread: https://webwork.maa.org/moodle/mod/forum/discuss.php?d=4790

Prior to this commit, Formula("-sqrt(x)") will render as -\left(\sqrt{x}\right) because the function application has lower precedence than negation. The commit changes that, so that this example renders as -\sqrt{x}. And similarly for any -function(x).

I can't think of a reason to keep it the way it is. The change seems to me to be an aesthetic improvement. And also a pedagogical improvement, since occasionally we get students wondering why WW is printing those parentheses, and asking if they are needed. Of course, I may be missing something.

@dpvc dpvc added the MathObject issue Issue involving MathObects code label Jan 3, 2021
@drgrice1
Copy link
Member

drgrice1 commented Mar 3, 2021

Is this something that we want to target for ww-2.16? I think it is a small change, and is a nice aesthetic improvement.

Note that there is another instance in which parentheses are rendered where they are not needed as well. That is the case that parserAssignment.pl is used. Things like x=-3 render as x=(-3). It would be nice to not have the parentheses there also.

Copy link
Member

@taniwallach taniwallach left a comment

Choose a reason for hiding this comment

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

The change works for me, and I think it is a good improvement.
I think this can be merged for WW 2.16.
@dvpc - Any comments on potential adverse effects of the change to precedence.

@taniwallach
Copy link
Member

I labeled as Do Not Merge Yet as I think any feedback from @dpvc about potential adverse consequences would be helpful before we do the merge. If he does not have time to look/comment - maybe someone else has some in-depth understanding of the potential implications of the precedence change.

@taniwallach
Copy link
Member

@drdrew42 @drgrice1 - what do you think about this?

Copy link
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 is a good change. @dpvc suggested it to @Alex-Jordan, although it would be good to get his official review. I am not sure how in depth he had thought it out when he suggested it.

@pstaabp pstaabp self-requested a review March 10, 2021 21:07
@pstaabp
Copy link
Member

pstaabp commented Mar 11, 2021

The develop branch needs to be merged in again to the PR. I'm not able to view anything because of the discrepancy between MathJax 2.7.X (which is in this PR) and 3.X which is develop.

@drgrice1
Copy link
Member

@pstaabp: You can merge the develop branch into the pull request branch after checking it out. It is not necessary to merge the develop branch into this pull request. That should become part of your workflow when testing pull requests.

@pstaabp
Copy link
Member

pstaabp commented Mar 11, 2021

@drgrice1 good call. I was trying figure out how I could test with Alex doing this.

@pstaabp
Copy link
Member

pstaabp commented Mar 11, 2021

Overall, looks good.

Is this something that we want to target for ww-2.16? I think it is a small change, and is a nice aesthetic improvement.

Note that there is another instance in which parentheses are rendered where they are not needed as well. That is the case that parserAssignment.pl is used. Things like x=-3 render as x=(-3). It would be nice to not have the parentheses there also.

I would agree with adding this as well, but we can do that at a later time.

I noted that parserImplicitEquation.pl on Formula("x=-3") returns x=-3 as expected.

Copy link
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.

I approve.

Copy link
Member

@dpvc dpvc left a comment

Choose a reason for hiding this comment

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

I don't think this should cause any problems.

@taniwallach taniwallach merged commit 20f4e8b into openwebwork:develop Mar 13, 2021
@dpvc
Copy link
Member

dpvc commented Mar 13, 2021

Note that there is another instance in which parentheses are rendered where they are not needed as well. That is the case that parserAssignment.pl is used. Things like x=-3 render as x=(-3). It would be nice to not have the parentheses there also.

This is unrelated to the function application precedence, and can be controlled by the showExtraParens flag:

Context()->flags->set(showExtraParens=>0);

but this will also remove other extra parentheses that make operator-operand connections painfully clear.

I've also made a pull request that will prevent these particular parentheses when parserAssignment.pl is used.

@drgrice1
Copy link
Member

I have tried setting the showExtraParens flag to 0 before (and just tested it again), and still get x=(-3). Your pull request does remove the additional parentheses though.

@dpvc
Copy link
Member

dpvc commented Mar 13, 2021

This test

loadMacros("parserAssignment.pl");
Context("Numeric")->flags->set(showExtraParens => 0); 
parser::Assignment->Allow;
TEXT(Formula("x = -3")->string);

produces x = -3 for me (without the PR), while with showExtraParens => 1 I get x = (-3).

@drgrice1
Copy link
Member

Yes, but if the student enters x=-3 as the answer it is displayed as x=(-3). For example using:

loadMacros("PGstandard.pl", "MathObjects.pl", "PGML.pl", "parserAssignment.pl");

Context("Numeric")->flags->set(showExtraParens => 0);
parser::Assignment->Allow;

$f1 = Formula("x = -3");

BEGIN_PGML
Enter [`[$f1]`]: [______]{$f1}
END_PGML

@dpvc
Copy link
Member

dpvc commented Mar 13, 2021

Sorry, the answer checker sets showExtraParens internally (it resets several flags during processing of student answers), so that is the reason.

@drgrice1
Copy link
Member

Yeah, I see that now. Well, your pull request works for student answers also.

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