changes to add root_notation flag in mathml.py. #15901

shiksha11 · 2019-02-02T10:48:13Z

Made Changes to fix issue #14033.
Added root_noatation flag in mathml.py.

>>>print_mathml(x**(S(1)/3), printer = 'presentation')
<mroot>
	<mi>x</mi>
	<mn>3</mn>
</mroot>

>>>print_mathml(x**(S(1)/3), printer = 'presentation', root_notation=False)
<msup>
	<mi>x</mi>
	<mfrac>
		<mn>1</mn>
		<mn>3</mn>
	</mfrac>
</msup>

printing
- Added the flag root_notation to the MathML printerchanges made in mathml.py. If set to False, exponents of the form 1/n are printed in fractional form. It is set as True (default), the exponents are printed in root form.

sympy-bot · 2019-02-02T10:48:27Z

✅

Hi, I am the SymPy bot (v136). 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:

printing
- Added the flag root_notation to the MathML printerchanges made in mathml.py. If set to False, exponents of the form 1/n are printed in fractional form. It is set as True (default), the exponents are printed in root form. (#15901 by @oscargus and @shiksha11)

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.

Made Changes to fix issue #14033.
Added root_noatation flag in mathml.py.

```javascript
>>>print_mathml(x**(S(1)/3), printer = 'presentation')
<mroot>
	<mi>x</mi>
	<mn>3</mn>
</mroot>

>>>print_mathml(x**(S(1)/3), printer = 'presentation', root_notation=False)
<msup>
	<mi>x</mi>
	<mfrac>
		<mn>1</mn>
		<mn>3</mn>
	</mfrac>
</msup>
```


<!-- BEGIN RELEASE NOTES -->
* printing
    * Added the flag root_notation to the MathML printerchanges made in mathml.py.  If set to False, exponents of the form 1/n are printed in fractional form. It is set as True (default), the exponents are printed in root form.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscargus · 2019-02-03T00:38:57Z

Thanks!

The printing part looks good, but I do not think the logic with the settings are working. (Maybe you were waiting for the init task to be solved?)

I have now merged #15906 which has a working init. If you merge this (will probably be easier if you remove both your init methods before merging master) you should be able to execute your code. Also, you can remove your default_setting.

Once that is done, please add a few tests to show that the flag changes the behavior. (Maybe it doesn't work in the content printer. Probably the settings handling should have been put there, but I was a bit too quick to merge #15906 I realize, as I didn't want to stall this PR.)

oscargus · 2019-02-03T01:02:03Z

sympy/printing/mathml.py

@@ -905,7 +920,7 @@ def mathml(expr, printer='content', **settings):
        return MathMLContentPrinter(settings).doprint(expr)


-def print_mathml(expr, printer='content', **settings):
+def print_mathml(expr, printer='content', symbol_names=None, root_notation=True, order=None):


It would be very useful if you can add all the settings from #15877 here. Starting with printer and order (to not break backwards compatibility) and then the rest in alphabetical order.

Even better if they are also added below, but here is the first priority.

okay, i am adding those settings.

oscargus · 2019-02-03T01:02:55Z

sympy/printing/mathml.py

@@ -112,6 +115,12 @@ class MathMLContentPrinter(MathMLPrinterBase):
    """
    printmethod = "_mathml_content"

+    def __init__(self, settings=None):


You can remove this and rely on the parent class instead I think.

oscargus · 2019-02-03T01:04:32Z

sympy/printing/mathml.py

@@ -466,6 +475,12 @@ class MathMLPresentationPrinter(MathMLPrinterBase):
    """
    printmethod = "_mathml_presentation"

+    def __init__(self, settings=None):
+        Printer.__init__(self, settings)


Here, it is better to call MathMLPrinterBase instead. However, this is just for future reference as this is already done in #15906. (Even better is super(MathMLPresentationPrinter, self).__init__(self, settings).)

shiksha11 · 2019-02-03T12:28:08Z

Thanks!

The printing part looks good, but I do not think the logic with the settings are working. (Maybe you were waiting for the init task to be solved?)

I have now merged #15906 which has a working init. If you merge this (will probably be easier if you remove both your init methods before merging master) you should be able to execute your code. Also, you can remove your default_setting.

Once that is done, please add a few tests to show that the flag changes the behavior. (Maybe it doesn't work in the content printer. Probably the settings handling should have been put there, but I was a bit too quick to merge #15906 I realize, as I didn't want to stall this PR.)

The changes i made work for content printer as well.

oscargus · 2019-02-03T15:24:24Z

Ah, so _settings is set in Printer.__init__? OK, then it should just be to resolve the conflicts and add a test and it should be good to go!

shiksha11 · 2019-02-04T08:08:29Z

Ah, so _settings is set in Printer.__init__? OK, then it should just be to resolve the conflicts and add a test and it should be good to go!

I have added root_notation in _default_settings of MathMLPrinterBase. Now, should i make the changes so that this flag works for both presentation or content printing or just presentation?

Abdullahjavednesar · 2019-02-04T08:36:10Z

@shiksha11 make sure you resolve the conflicting file and add a test to support the changes you've made before you proceed ahead.
@oscargus thanks for the review 🙂

oscargus · 2019-02-04T22:26:30Z

sympy/printing/mathml.py

@@ -466,7 +467,7 @@ class MathMLPresentationPrinter(MathMLPrinterBase):

    def __init__(self, settings=None):
        MathMLPrinterBase.__init__(self, settings)
-
+        


Here are some spaces.

removed those spaces.
Are there any tests which i can run locally to check for these errors related to spaces or code style?

Yes, python bin\test quality on "Windows", bin/test quality on "Linux". (Citation marks as exact usage will vary depending on so many things...)

Takes less than a minute so clearly worth it.

Okay, thanks :)

It is also possible to run a subset of the tests by replacing quality with the module name, in this case printing. One can also run only a specific test with the -k flag, in your case -k test_presentation_printmethod (using printingas well will probably speed things up as not all tests are searched).

The problem now is that the flag is passed when instantiating the printer, not in the doprint method. So you will need to instantiate a new printer with the flag. Probably makes sense to move it to a separate test as well to make it clear that it uses a different printer instantiation.

Yes, i was initially unsure about adding the test in the doprint method.

oscargus · 2019-02-05T16:11:59Z

I added the missing import. If the tests pass it should be good to go. Thanks!

oscargus · 2019-02-06T08:37:15Z

The problem now is that the flag is passed when instantiating the printer, not in the doprint method. So you will need to instantiate a new printer with the flag. Probably makes sense to move it to a separate test as well to make it clear that it uses a different printer instantiation.

oscargus · 2019-02-06T08:42:22Z

I added that now.

Primarily to get a new Azure run, which seems to have not started last time.

shiksha11 · 2019-02-06T12:59:52Z

I added that now.

Thanks!!

shiksha11 · 2019-02-06T13:27:47Z

The problem now is that the flag is passed when instantiating the printer, not in the doprint method. So you will need to instantiate a new printer with the flag. Probably makes sense to move it to a separate test as well to make it clear that it uses a different printer instantiation.

Did you understand the problem from the error messages ?

oscargus · 2019-02-06T13:47:13Z

Yes.

From https://travis-ci.org/sympy/sympy/jobs/488910309

______ sympy/printing/tests/test_mathml.py:test_presentation_printmethod _______
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/utilities/runtests.py", line 1306, in _timeout
    function()
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/printing/tests/test_mathml.py", line 464, in test_presentation_printmethod
    assert mpp.doprint(x**(S(1)/3)) == '<mroot><mi>x</mi><mn>3</mn></mroot>'
NameError: name 'S' is not defined

and later from https://travis-ci.org/sympy/sympy/jobs/489118673

______ sympy/printing/tests/test_mathml.py:test_presentation_printmethod _______
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/utilities/runtests.py", line 1306, in _timeout
    function()
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/printing/tests/test_mathml.py", line 465, in test_presentation_printmethod
    assert mpp.doprint(x**(S(1)/3), root_notation=False) == '<msup><mi>x</mi><mfrac><mn>1</mn><mn>3</mn></mfrac></msup>'
TypeError: doprint() got an unexpected keyword argument 'root_notation'

Not sure why the Azure tests are not running though...

shiksha11 · 2019-02-07T06:00:14Z

Yes.

From https://travis-ci.org/sympy/sympy/jobs/488910309

______ sympy/printing/tests/test_mathml.py:test_presentation_printmethod _______
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/utilities/runtests.py", line 1306, in _timeout
    function()
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/printing/tests/test_mathml.py", line 464, in test_presentation_printmethod
    assert mpp.doprint(x**(S(1)/3)) == '<mroot><mi>x</mi><mn>3</mn></mroot>'
NameError: name 'S' is not defined

and later from https://travis-ci.org/sympy/sympy/jobs/489118673

______ sympy/printing/tests/test_mathml.py:test_presentation_printmethod _______
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/utilities/runtests.py", line 1306, in _timeout
    function()
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/sympy-1.4.dev0-py3.6.egg/sympy/printing/tests/test_mathml.py", line 465, in test_presentation_printmethod
    assert mpp.doprint(x**(S(1)/3), root_notation=False) == '<msup><mi>x</mi><mfrac><mn>1</mn><mn>3</mn></mfrac></msup>'
TypeError: doprint() got an unexpected keyword argument 'root_notation'

Not sure why the Azure tests are not running though...

Thanks! Now i understand how to interpret those messages.

oscargus · 2019-02-07T11:39:06Z

I merge it now. Thanks!

shiksha11 and others added 2 commits February 2, 2019 15:50

added root_notation flag in mathml.py

128853d

made root_notation true by default.

066f9b3

shiksha11 changed the title ~~changes to add root_notation flag in math.py.~~ changes to add root_notation flag in mathml.py. Feb 2, 2019

Abdullahjavednesar requested a review from oscargus February 2, 2019 18:26

oscargus mentioned this pull request Feb 3, 2019

Add __init__ to MathMLPresentationPrinter #15906

Merged

oscargus mentioned this pull request Feb 3, 2019

Add support for configuration of the MathML printers #15877

Open

oscargus reviewed Feb 3, 2019

View reviewed changes

oscargus added the printing label Feb 3, 2019

shiksha11 added 2 commits February 4, 2019 19:52

Merge branch 'master' into newbranch2

032c1c6

added test for root_notation flag.

741742e

oscargus reviewed Feb 4, 2019

View reviewed changes

shiksha11 and others added 3 commits February 5, 2019 13:22

removed white space

e43f214

Added import of S

7be4d2b

Added import of S. Actually. No fat fingers this time.

6581e26

Fixed test.

5dcfec5

oscargus added 2 commits February 6, 2019 10:55

How hard can it be? Fixed printer call.

a4489d7

Cleanup and added root_notation to content printer

ed2a659

Primarily to get a new Azure run, which seems to have not started last time.

oscargus merged commit d05637a into sympy:master Feb 7, 2019

shiksha11 deleted the newbranch2 branch February 8, 2019 15:13

oscargus mentioned this pull request Feb 13, 2019

Add a flag root_notation to relevant printers #14033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes to add root_notation flag in mathml.py. #15901

changes to add root_notation flag in mathml.py. #15901

shiksha11 commented Feb 2, 2019 •

edited by oscargus

sympy-bot commented Feb 2, 2019 •

edited

oscargus commented Feb 3, 2019

oscargus Feb 3, 2019

shiksha11 Feb 3, 2019

oscargus Feb 3, 2019

oscargus Feb 3, 2019

shiksha11 Feb 3, 2019

shiksha11 commented Feb 3, 2019

oscargus commented Feb 3, 2019

shiksha11 commented Feb 4, 2019

Abdullahjavednesar commented Feb 4, 2019

oscargus Feb 4, 2019

shiksha11 Feb 5, 2019

oscargus Feb 5, 2019

shiksha11 Feb 5, 2019

oscargus Feb 5, 2019

shiksha11 Feb 6, 2019

oscargus commented Feb 5, 2019

oscargus commented Feb 6, 2019

oscargus commented Feb 6, 2019

shiksha11 commented Feb 6, 2019

shiksha11 commented Feb 6, 2019 •

edited

oscargus commented Feb 6, 2019

shiksha11 commented Feb 7, 2019

oscargus commented Feb 7, 2019

		@@ -466,7 +467,7 @@ class MathMLPresentationPrinter(MathMLPrinterBase):

		def __init__(self, settings=None):
		MathMLPrinterBase.__init__(self, settings)

changes to add root_notation flag in mathml.py. #15901

changes to add root_notation flag in mathml.py. #15901

Conversation

shiksha11 commented Feb 2, 2019 • edited by oscargus

sympy-bot commented Feb 2, 2019 • edited

oscargus commented Feb 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiksha11 commented Feb 3, 2019

oscargus commented Feb 3, 2019

shiksha11 commented Feb 4, 2019

Abdullahjavednesar commented Feb 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Feb 5, 2019

oscargus commented Feb 6, 2019

oscargus commented Feb 6, 2019

shiksha11 commented Feb 6, 2019

shiksha11 commented Feb 6, 2019 • edited

oscargus commented Feb 6, 2019

shiksha11 commented Feb 7, 2019

oscargus commented Feb 7, 2019

shiksha11 commented Feb 2, 2019 •

edited by oscargus

sympy-bot commented Feb 2, 2019 •

edited

shiksha11 commented Feb 6, 2019 •

edited