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
added the fold_frac_powers and fold_short_frac flag to mathml printer #16075
added the fold_frac_powers and fold_short_frac flag to mathml printer #16075
Conversation
✅ Hi, I am the SymPy bot (v142). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.4. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
I haven't added the test cases till now. Once all changes are made then I will add them. |
sympy/printing/mathml.py
Outdated
@@ -554,6 +554,8 @@ def multiply(expr, mrow): | |||
numer, denom = fraction(expr) | |||
if denom is not S.One: | |||
frac = self.dom.createElement('mfrac') | |||
if self._settings["fold_short_frac"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should one make some sort of length test here? Or does one already know that the length is short?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. I forgot it there.
sympy/printing/mathml.py
Outdated
x.appendChild(self._print(e.exp)) | ||
return x | ||
if e.exp.is_negative: | ||
if self._settings['fold_short_frac'] and len(str(e.base)) < 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please provide an example of MathML code generated from this part of the code? I cannot really figure out what it looks like (and when this happens).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In [3]: a= 1/(x)
In [4]: print_mathml(a, printer='presentation', fold_short_frac=True)
<mfrac bevelled="true">
<mn>1</mn>
<msup>
<mrow>
<mfenced>
<mi>x</mi>
</mfenced>
</mrow>
<mn>1</mn>
</msup>
</mfrac>
Is this how you want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I am not sure if this should happen or not. Well, this particular example shouldn't happen, but in general.
Here, you will need to check if the exponent is one and in that case not print the exponent, now it looks like
There is a method in another PR to check if there should be brackets around the expression, now maybe one could check if it e.base
is Symbol
and if it is not print the bracket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which PR is that to check for e.base
is Symbol
? or to check for brackets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bracket check is actually merged. I didn't realize that (I have two outstanding PRs on this and I though it was in one of those). See #16069
I have also realized that the behavior of the MathML Rational is completely different to the LaTeX one, so as long as the 1-exponent is not printed I think this would be good to go for now. One can always come back and make it more like the LaTeX one if that is preferred.
I think it looks good! Just some minor comments for now. Please add some tests as it will be easier to see the effect of the change then. |
sympy/printing/mathml.py
Outdated
@@ -554,6 +554,9 @@ def multiply(expr, mrow): | |||
numer, denom = fraction(expr) | |||
if denom is not S.One: | |||
frac = self.dom.createElement('mfrac') | |||
print(expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh sorry. That was by mistake. It was just for testing purpose.
sympy/printing/mathml.py
Outdated
mrow.appendChild(self._print(e.base)) | ||
frac.appendChild(mrow) | ||
return frac | ||
if type(e.base) == symbol.Symbol: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that you can do e.base.is_symbol
(correct syntax would otherwise by e.base is Symbol
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh Yeah. It's working.
Can Anyone merge this PR? I want to make another PR. |
I think the added functionality makes sense. A problem is that the original MathML printer is inconsistent with the LaTeX printer. Hence, I merge this for now. @shubhamabhang77 it makes sense to always start a new branch/PR/functionality from master, unless you are actually using some code from a previous PR. There may still be some issues with the other PR even after merging this. |
Tell me if there are any changes that has to be. |
I wil try to prepare some things related to that now that the functionality is in. The main thing is really that the LaTeX and MathML presentation printers prints things like 1/y**2 differently. But that is really nothing that affects this PR. |
I made another branch but I don't know what happened when I opened another PR, commits from PR was also listed there. |
References to other Issues or PRs
Brief description of what is fixed or changed
Other comments
Release Notes
printing