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

reduce incorrectly removes parenthesis in mathobject string. #634

Closed
somiaj opened this issue Jan 18, 2022 · 11 comments
Closed

reduce incorrectly removes parenthesis in mathobject string. #634

somiaj opened this issue Jan 18, 2022 · 11 comments
Labels
MathObject issue Issue involving MathObects code

Comments

@somiaj
Copy link
Contributor

somiaj commented Jan 18, 2022

When using reduce (with a double negative) parenthesis are removed from the ->string. Here is my test case

DOCUMENT();
loadMacros(
  "PGstandard.pl",
  "PGML.pl",
);
TEXT(beginproblem());

$a = Formula("-4x^2")->reduce;
$test1 = Formula("e^(-$a)")->reduce;
$test2 = Formula("e^(-$a)");

BEGIN_PGML
**Test 1:**
+ String: [$test1->string]
+ TeX: [``[$test1]``]

**Test 2:**
+ String: [$test2->string]
+ TeX: [``[$test2]``]
END_PGML

ENDDOCUMENT();

This causes the ->string to loose the parenthesis , but ->TeX is fine.

Screenshot if issue.

I think this is related to a similar bug, but it wasn't fully fixed in issue #455 and the pull request #546.

@dpvc
Copy link
Member

dpvc commented Jan 18, 2022

This turns out to be a bad interaction between two different reduction rules. I have made a pull request to fix it (linked above).

@dpvc dpvc added the MathObject issue Issue involving MathObects code label Jan 18, 2022
@somiaj
Copy link
Contributor Author

somiaj commented Jan 18, 2022

Thanks, I can confirm this fixes the issue.

@dpvc I notice you only have a pull request for develop, this bugfix seems reasonable to fix in main as well (imo).

@dpvc
Copy link
Member

dpvc commented Jan 18, 2022

I will let others make that call. I'm not up on the release process any more. If so, then the PR should be rebased to main and targeted there, and then targeted to develop as well. But that may not be how things are done any longer.

@drgrice1
Copy link
Member

@dpvc: That is pretty much it. Rebase and retarget.

I am not sure if this should be a hotfix or not. I am not certain that is so pressing. It is really a display issue, and those are usually things that we defer to the next release.

What do others think?

@somiaj
Copy link
Contributor Author

somiaj commented Jan 18, 2022

@drgrice1 The reason I think it should be put in main is because although the original mathobject is correct, the string is used to build future mathobjects, so any mathobject built using this mathobject, such as $test3 = Formula("5 + $test1") will be incorrect (which is how I ran across this issue in the first place). I can patch locally, I just thought this might be pressing enough due to this.

@dpvc
Copy link
Member

dpvc commented Jan 18, 2022

The only time it would be a computation issue is if the reduced function is then inserted into another string (for Compute() for example), then the result would be wrong. But as it has been that way for 14 years (that is when the noParens section was added), I suspect it is not crucial. I see @somiaj pointed out the same issue with using it in alternative strings. Of course, his example could be done as $test3 = 5 + $test1 without causing error, other examples are harder to handle that way without a little more care.

@drgrice1
Copy link
Member

As @dpvc pointed out, it has been that way for a long time. We can't just run to put in a hotfix every time a minor issue comes up.

It won't work in further computations, so work around it for now. There are other ways to do it.

@somiaj
Copy link
Contributor Author

somiaj commented Jan 18, 2022

Thanks for the clarification.

@drgrice1
Copy link
Member

Again, I am waiting on other opinions. So if the general consensus is that this is hotfix worthy, then we will put it in.

@pstaabp
Copy link
Member

pstaabp commented Jan 18, 2022

I think, also, we can wait for the next release. I'm not sure how many people pull in hot fixes to their servers anyway and it's not a security bug. @somiaj , maybe make a post on the forum discussing this so people are aware.

pstaabp added a commit that referenced this issue Feb 5, 2022
Remove noParens marker from operand (if any) when reducing double negatives. #634
@somiaj
Copy link
Contributor Author

somiaj commented Feb 12, 2022

Fixed with #635.

@somiaj somiaj closed this as completed Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MathObject issue Issue involving MathObects code
Projects
None yet
Development

No branches or pull requests

4 participants