numpy icon indicating copy to clipboard operation
numpy copied to clipboard

MAINT: Make output of Polynomial representations consistent

Open eendebakpt opened this issue 3 years ago • 7 comments

This is a followup from #21653 and #21654. In this PR we make the output of Polynomial.__str__ and Polynomial._repr_latex_ consistent.

Input

import numpy as np
x = np.arange(10)
y = 5 * x + 1 + .1*x*x-.1*x*x*x
f = np.polynomial.Polynomial.fit(x, y, 1)

print(str(f))
print(f._repr_latex_())

np.set_printoptions(precision=2)
print(str(f))
print(f._repr_latex_())

Output before:

6.1 - 7.38 x
$x \mapsto \text{6.1} - \text{7.38}\,\left(\text{-1.0} + \text{0.22222222}x\right)$
6.1 - 7.38 x
$x \mapsto \text{6.1} - \text{7.38}\,\left(\text{-1.0} + \text{0.22}x\right)$

After:

6.1 - 7.38 (-1.0 + 0.22222222x)
$x \mapsto \text{6.1} - \text{7.38}\,\left(\text{-1.0} + \text{0.22222222}x\right)$
6.1 - 7.38 (-1.0 + 0.22x)
$x \mapsto \text{6.1} - \text{7.38}\,\left(\text{-1.0} + \text{0.22}x\right)$

eendebakpt avatar Jun 14 '22 20:06 eendebakpt

@rossbar maybe you can have a look? @eendebakpt this change looks like it should get a basic tests.

seberg avatar Jun 15 '22 12:06 seberg

I would also suggest adding \cdot in the LaTeX representation. Currently it is displayed in the str() but not in the display():

image

Basically instead of image it would display as image

axil avatar Jun 16 '22 16:06 axil

The necessary change is trivial:

diff --git a/numpy/polynomial/_polybase.py b/numpy/polynomial/_polybase.py
index 920377535..483068cb5 100644
--- a/numpy/polynomial/_polybase.py
+++ b/numpy/polynomial/_polybase.py
@@ -490,7 +490,7 @@ def _repr_latex_(self):
             if term_str == '1':
                 part = coef_str
             else:
-                part = rf"{coef_str}\,{term_str}"
+                part = rf"{coef_str}\cdot{term_str}"

             if c == 0:
                 part = mute(part)

I can take care of the tests if the change is approved.

axil avatar Jun 16 '22 16:06 axil

Also in ascii mode:

>>> print(str(f))
23.98618858 + 23.21718647*(-1.0 + 0.22222222x) -
0.70392674*(-1.0 + 0.22222222x)**2 - 1.23554729*(-1.0 + 0.22222222x)**3
diff --git a/numpy/polynomial/polynomial.py b/numpy/polynomial/polynomial.py
index 8e2c6f002..2cb96ee5d 100644
--- a/numpy/polynomial/polynomial.py
+++ b/numpy/polynomial/polynomial.py
@@ -1520,9 +1520,9 @@ def _str_term_unicode(cls, i, arg_str):
     @staticmethod
     def _str_term_ascii(i, arg_str):
         if i == '1':
-            return f" {arg_str}"
+            return f"*{arg_str}"
         else:
-            return f" {arg_str}**{i}"
+            return f"*{arg_str}**{i}"

     @staticmethod
     def _repr_latex_term(i, arg_str, needs_parens):

axil avatar Jun 16 '22 17:06 axil

Or maybe to be verbose about the '*' before 'x' as well

>>> print(str(f))
23.98618858 + 23.21718647*(-1.0 + 0.22222222*x) -
0.70392674*(-1.0 + 0.22222222*x)**2 - 1.23554729*(-1.0 + 0.22222222*x)**3

(in ascii mode only), so that after copy-paste it would be a valid python string that can be executed with no modifications.

axil avatar Jun 16 '22 17:06 axil

@axil Personally, I do not have a strong opinion on whether to use a dot, asterisk or no multiplication symbol (in either latex or str). But I am a bit hesitant to make changes to the current implementation.

If there is consensus with another numpy dev I would be fine with making the changes. Or perhaps make quick poll on the numpy mailing list and decide which format to pick

eendebakpt avatar Jun 16 '22 20:06 eendebakpt

@rossbar maybe you can have a look? @eendebakpt this change looks like it should get a basic tests.

Basic test was added.

@rossbar Could you have a look at this PR?

eendebakpt avatar Sep 17 '22 19:09 eendebakpt

@seberg Do you want me to write release notes for this PR? I guess it should be with the compatibility label.

eendebakpt avatar Dec 11 '22 12:12 eendebakpt

@seberg @rossbar Release notes have been added. Let now if I can do anything else for the PR.

eendebakpt avatar Mar 10 '23 09:03 eendebakpt

We discussed that in the triage meeting and decided that either @charris or @rossbar should just make a decision on the dot bikeshed :).

seberg avatar May 31 '23 17:05 seberg

@charris or @rossbar, could one of you review this PR?

eendebakpt avatar Aug 30 '23 20:08 eendebakpt

@eendebakpt seems this created a merge conflict (failing tests). Any chance you can look into that?

seberg avatar Mar 07 '24 10:03 seberg

I am seeing a new error after this was merged on some unrelated PR:

    def test_polynomial_str(self):
        res = str(poly.Polynomial([0, 1]))
        tgt = '0.0 + 1.0 x'
>       assert_equal(res, tgt)
E       AssertionError: 
E       Items are not equal:
E        ACTUAL: '0.0 + 1.0·x'
E        DESIRED: '0.0 + 1.0 x'

I wonder how this passed, maybe it was not merged to main first?

mattip avatar Mar 07 '24 11:03 mattip

I will look into it

eendebakpt avatar Mar 07 '24 11:03 eendebakpt

This PRwas merged without merging with main first it seems. The latest time CI was executed was 5 months ago, so that might explains why we see some errors now.

The ABCPolyBase has an attribute _use_unicode = not os.name == 'nt' that determines whether we can have the dot symbol in the represenation or not. The test from @mattip works if _use_unicode is False, but fails if _use_unicode is True.

eendebakpt avatar Mar 07 '24 12:03 eendebakpt