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

Properly add parentheses to Fraction::Value objects based on precedence. #512

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Dec 9, 2020

Currently, Fraction MathObjects may lose necessary parentheses when stringified or texified. For example Compute("x^(2/7)")->string; will produce x^2/7. This means that

$f = Compute("x^(2/7)");
$g = Compute("$f");

would produce $g equivalent to Compute("(x^2)/7") rather than equivalent to $f. (See this Forum post for more details.)

This PR resolves the issue by overriding the string() and TeX() methods to properly insert parentheses when needed. These are added when the operator precedences require it, or (in string()) when the original had them (since the TeX version is a vertical fraction, they are are only added when precedence requires it).

To test, use

loadMacros("PGML.pl", "contextFraction.pl");

sub LaTeX {
  my $f = shift;
  return PGML::LaTeX('\(' . $f->TeX . '\)');
};

Context("Fraction");

$f1 = Compute("x^(2/7)");
$f2 = Compute("(2/7)*x");
$f3 = Compute("2/7 x");

TEXT($f1, " has parens", $BR);
TEXT(Compute("$f1"), " has parens", $BR);
TEXT($f2, " has parens", $BR);
TEXT(Compute("$f2"), " has parens", $BR);
TEXT($f3, " has no parens", $BR);
TEXT(Compute("$f3"), " has no parens", $BR);
TEXT($HR);

$x = Formula("x");
$f = Fraction(2,7);

$f4 = $x ** $f;
$f5 = $f * $x;

TEXT($f4, " has parens", $BR);
TEXT(Compute("$f4"), " has parens", $BR);
TEXT($f5, " has no parens", $BR);
TEXT(Compute("$f5"), " has no parens", $BR);

TEXT($HR, "No parens:", $BR);
TEXT(LaTeX($f1), $BR);
TEXT(LaTeX(Compute("$f1")), $BR);
TEXT(LaTeX($f2), $BR);
TEXT(LaTeX(Compute("$f2")), $BR);
TEXT(LaTeX($f3), $BR);
TEXT(LaTeX(Compute("$f3")), $BR);

TEXT($HR, "No parens:", $BR);
TEXT(LaTeX($f4), $BR);
TEXT(LaTeX(Compute("$f4")), $BR);
TEXT(LaTeX($f5), $BR);
TEXT(LaTeX(Compute("$f5")), $BR);

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.

Thanks for fixing this. Looks good.

@Alex-Jordan Alex-Jordan merged commit 438d97c into openwebwork:develop Dec 16, 2020
@Alex-Jordan
Copy link
Contributor

I tested as well, and it works as expected.

@dpvc dpvc deleted the fix-fraction-parens branch December 16, 2020 01:39
@drdrew42
Copy link
Member

I didn't have time to deep-dive, but look at the differences between:

  • Fraction(1,2) -- no Function wrapper
  • Formula(Fraction(1,2)) vs Formula("1/2")
  • what changes with Context('Fraction')->flags->set(reduceConstants=>0)

@Alex-Jordan
Copy link
Contributor

  • Making a Fraction object (Fraction(1,2) or Fraction("1/2") or Fraction(1/2)), there are no extra parens. It's only happening with Formula objects.
  • Any way I make a Formula object in the Fraction context, I get extra parens. Both of these Formula(Fraction(1,2)) and Formula("1/2") end up displaying extra parens.
  • You hit on something: using Context('Fraction')->flags->set(reduceConstants=>0) does leave off the excess parens. It's not a solution but that's an interesting clue.
  • I tried setting the flag showExtraParens=>0, but that is not helping.

@drdrew42
Copy link
Member

drdrew42 commented Oct 13, 2021

After further testing, when seeing double-parens, the outer parens are from this commit. (So in this example, $neg with reduceConstants=>1)

The parens that appear for $pos (and the inner-parens on $neg) are due to the changes in this commit: 77c45ea from #569 (stemming from issue #567)

Also note: Formula("1/2") and Formula(Fraction(1,2)) do render differently when reduceConstants=>0. (Parens appearing here are also due to that other commit...)

@drdrew42
Copy link
Member

drdrew42 commented Oct 14, 2021

Furthermore, the other PR only corrects the use of input parameters to the TeX subroutine -- so presumably precedence is now actually being handled properly.

Also, look at the difference in behavior for the non-TeX strings $pos and $neg = Formula("-$pos")

MathObject $pos reduceConstants $pos string parens $pos TeX parens $neg string parens $neg TeX parens
Formula("1/2") Y 2 1 2 2
Formula("1/2") N 0 0 1 0
Formula(Fraction(1,2)) Y 1 1 2 2
Formula(Fraction(1,2)) N 1 1 1 0

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.

4 participants