Skip to content

Fix calls to Parser::Value::TeX & too many (cooks)#608

Merged
drgrice1 merged 4 commits intoopenwebwork:mainfrom
drdrew42:bugfix/too-many-fraction-parens
Dec 2, 2021
Merged

Fix calls to Parser::Value::TeX & too many (cooks)#608
drgrice1 merged 4 commits intoopenwebwork:mainfrom
drdrew42:bugfix/too-many-fraction-parens

Conversation

@drdrew42
Copy link
Member

@drdrew42 drdrew42 commented Oct 15, 2021

In PR #512, contextFraction.pl inserted new methods into the string and TeX method call chains (originally routed directly to Parser::Value::TeX, now passing through context::Fraction::Value). These methods had the impact of adding necessary parens to the string (as precedence would require -- see reference).

However, strictly following precedence at the Fraction::Value layer causes issues with unary negation (as reported by @Alex-Jordan) -- because the precedence of u- is 6 (and precedence of / is 3).

This issue is avoided in Fraction::BOP::divide::TeX via argument $showparens eq 'nofractions' via UOP::TeX (derived from Parser::Context::Default::operations->{'u-'}{nofractionparens}). This solution is not available at the Fraction::Value layer (and is already being handled on the BOP layer), so these additional parens will now only be added when either:

  • the parens are explicitly there in the original string definition, or
  • precedence dictates AND we've explicitly set showExtraParens => 2 in the context

Meanwhile the precedence parens remain a necessary addition for strings at the Fraction::Value layer because we cannot be assured that the original string contained them (see attached pg example of constructing a MathObject with precedence derived from perl operations [modified from code provided in #512]).

Furthermore, subsequent changes in #569 (stemming from issue #567) changed context::Fraction::Fraction::TeX to properly handle the incoming precedence. This had the effect of yet another layer trying to insert string and TeX parens based on precedence. This code was not previously being acted upon because the incoming precedence was not being handled properly (Fraction objects should never have ->{open} or ->{closed} defined as noted by @taniwallach). This is definitely not the layer to be handling precedence issues and so that code has been removed.

See attached code: check-parens.pg.txt

@drdrew42 drdrew42 requested a review from dpvc October 15, 2021 04:05
@pstaabp
Copy link
Member

pstaabp commented Oct 15, 2021

This does seem to fix the problem with the unary minus and fractions. We don't have to do this here, but it seems logical to not have parens at all around either 1/2 or -1/2. For example:

Formula("-((x+3)/(x-4))");

returns $$-\frac{x+3}{x-4}$$

which is the way I would write it. Throw these cases into the unit-test that I will write one day. :)

@drdrew42 drdrew42 requested a review from Alex-Jordan October 15, 2021 22:30
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.

This looks good. Although, as it is only a display issue, I am not sure that this should be a hot fix to main.

@Alex-Jordan
Copy link
Contributor

Thanks for jumping on this and figuring out. I tried, but it was giving me a headache. I can report that the additional parentheses around a positive fraction are gone now, as advertised. And your test file works.

Do I understand right that this doesn't attempt to address the parentheses around a negated fraction?

@drgrice1 I see where you are coming from. However for what it's worth, we've had students writing to ask if the double parentheses have some special meaning they haven't learned. So in a certain pedagogical sense, the issue is affecting WeBWorK functionality. But either way. If it doesn't go to main right now, I can just apply the patch to my school's server.

@drgrice1
Copy link
Member

I am fine with it going in as a hot fix. However, this is the sort of thing that would often be deferred to the next release.

@drdrew42
Copy link
Member Author

Additional change:

Before

Parser sets the hadParens flag at the very top level of the parsed formula. This is why we see $pos = Formula("1/2") receiving parens, and then they are inherited by Formula("-$pos") which parses $pos->string (generated by the MWE provided here). Notice that the same is not true for $pos = Fraction(1,2) and Formula("-$pos") because we're not parsing a string for $pos (and when parsing "-$pos", the top-level hadParens flag is being set on the UOP, rather than on the Fraction).

After

After this additional commit, we should see the same TeX from Formula("1/2") and Fraction(1,2) regardless of reduceConstants. The same should hold for Formula("-$pos"). Furthermore, the string for all of these versions of 1/2 is also consistent (with or without reduceConstants).

This additional change should have no other effect, since the hadParens flag is only used in contextFraction.pm and contextAlternativeIntervals.pm. In contextAlternativeIntervals, the presence of the hadParens flag is contained in a duplication of the code block from Parser that we're editing now, so this change should have no effect there.

@Alex-Jordan I think this latest change should address the negated fractions.

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 have made a suggested edit, but otherwise looks fine.

Co-authored-by: Davide P. Cervone <dpvc33@gmail.com>
@drgrice1 drgrice1 merged commit 76ba414 into openwebwork:main Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants