MAINT: Legacy mode specified as string, fix all-zeros legacy bug #10030

ahaldane · 2017-11-15T19:40:09Z

The first commit makes the legacy option specify version number, as np.set_printoptions(legacy='1.13').

The second commit fixes a bug where the legacy mode spacing for float arrays didn't get activated for arrays which didn't have any non-zero finite values.

eric-wieser · 2017-11-16T08:44:10Z

numpy/core/arrayprint.py

@@ -87,6 +87,9 @@ def _make_options_dict(precision=None, threshold=None, edgeitems=None,
    if sign not in [None, '-', '+', ' ']:
        raise ValueError("sign option must be one of ' ', '+', or '-'")

+    if legacy not in [None, '', '1.13']:
+        raise ValueError("legacy option can currently only be '1.13' or ''")


I think this should warn, not error - otherwise, code which does not want 1.15 behavior (passing legacy='1.14'), will crash when run on 1.13.

eric-wieser · 2017-11-16T08:45:19Z

Unsure if we should be using strings or tuples here. Tuples would work better for version comparison, but perhaps that's not something we want to encourage.

lesteve · 2017-11-16T09:25:31Z

Unsure if we should be using strings or tuples here. Tuples would work better for version comparison, but perhaps that's not something we want to encourage.

My 2c: use strings and distutils.version.LooseVersion. As long as you are comparing released versions, i.e. X.Y.Z (and not things like 1.14.dev0 or 1.14.1b1) distutils.LooseVersion is good enough. You can also LooseVersion('1.1.0').version to get a list of integers corresponding to the major/minor/micro versions.

charris · 2017-11-16T14:20:27Z

@lesteve We don't need to worry about actual versions, just need a way to specify which printing style we want. I'd go for a string.

ahaldane · 2017-11-16T19:21:36Z

You're right to warn, instead of raise.

Fixed.

eric-wieser · 2017-11-17T05:21:49Z

numpy/core/arrayprint.py

@@ -87,6 +87,10 @@ def _make_options_dict(precision=None, threshold=None, edgeitems=None,
    if sign not in [None, '-', '+', ' ']:
        raise ValueError("sign option must be one of ' ', '+', or '-'")

+    if legacy not in [None, '', '1.13']:
+        warnings.warn("legacy printing option can currently only be '1.13' or "
+                      "''", stacklevel=3)


Empty string seems weird to me. Can we use False instead? I'm guessing None is a problem because of needing a default value in set_printoptions?

eric-wieser · 2017-11-17T05:29:26Z

numpy/core/arrayprint.py

-            valid = not_equal(data, 0) & ~special
-            non_zero = data[valid]
+            non_special = data[~special]
+            non_zero = non_special[non_special != 0]


Some comments might be nice here explaining why each of these are needed.

A 'not' prefix has more force than 'non'. Not worth worrying about here, just a comment.

I would reserve a not_ prefix for a boolean type, so non_zero = data[not_zero]

Fixes numpy#10020

Fixes numpy#10026

ahaldane · 2017-11-17T21:23:35Z

Thanks for the suggestions. I made legacy=False disable legacy mode, and I did a lot of cleanup in the FloatingFormat code in a new commit.

ahaldane · 2017-11-17T21:27:28Z

numpy/core/arrayprint.py

@@ -841,7 +834,7 @@ def __init__(self, *args, **kwargs):
 def format_float_scientific(x, precision=None, unique=True, trim='k',
                            sign=False, pad_left=None, exp_digits=None):
    """
-    Format a floating-point scalar as a string in scientific notation.
+    Format a floating-point scalar as a decimal string in scientific notation.


this is an unrelated small doc tweak I am piggybacking on this PR

charris · 2017-11-18T01:31:33Z

numpy/core/arrayprint.py

+        If set to the string `'1.13'` enables 1.13 legacy printing mode. This
+        approximates numpy 1.13 print output by including a space in the sign
+        position of floats and different behavior for 0d arrays. If set to
+        `False`, disables legacy mode.



Needs .. versionadded::. If folks want to use this they will need to check the NumPy version.

Might want a comment here explaining that invalid versions will be ignored for forward-compatibility too.

Don't forget to add this to both docstrings

eric-wieser · 2017-11-18T01:36:22Z

numpy/core/arrayprint.py

+        finite_vals = data[isfinite(data)]
+
+        # choose exponential mode based on the non-zero finite values:
+        abs_non_zero = absolute(finite_vals[finite_vals != 0])


Way clearer now :)

@eric-wieser If you are good with this, I'll make the fix and merge.

eric-wieser · 2017-11-18T01:52:43Z

numpy/core/arrayprint.py

@@ -701,40 +707,32 @@ def __init__(self, data, precision, floatmode, suppress_small, sign=False, **kwa
        self.fillFormat(data)

    def fillFormat(self, data):
-        with errstate(all='ignore'):


Why did this line go away?

Oh I think I need to keep it. It must be because the division operation can overflow, eg 1.0/np.float64(2.2250738585072e-310).

Oops, I just made the other fixes online.

You may need to update from origin.

Maybe push it to a deeper scope this time

eric-wieser · 2017-11-18T01:54:57Z

A little puzzled by the errstate(all='ignore') vanishing, but all looks good to me modulo the doc fix you spotted.

eric-wieser

LGTM. Might be nice to squash @charris' commits together

eric-wieser · 2017-11-18T02:50:34Z

numpy/core/tests/test_arrayprint.py

+    def test_float_overflow_nowarn(self):
+        # make sure internal computations in FloatingFormat don't
+        # warn about overflow
+        repr(array([1.0, 1e-310], dtype='f8'))


Hmm but actually tests just finished running on my system and I don't think this triggers the problem because the min_val < 0.0001 short-circtuits out the division clause anytime overflow could occur.

I think overflow may only be posible for float16 types under certain circumstances. np.array([1e4, 0.1], dtype='f2') works.

Note that the `legacy` keyword was added in NumPy 1.14.0. [ci skip]

charris · 2017-11-18T03:34:31Z

The errstate block seems to have originated back around 2007, although not implemented that way as errstate was a later addition.

charris · 2017-11-18T16:44:01Z

Thanks Allan.

ahaldane added 03 - Maintenance component: numpy._core labels Nov 15, 2017

ahaldane added this to the 1.14.0 release milestone Nov 15, 2017

ahaldane changed the title ~~Legacy mode fixes~~ MAINT: Legacy mode fixes Nov 15, 2017

This was referenced Nov 15, 2017

MAINT: Remove newline before dtype in repr of arrays #10032

Merged

MAINT: legacy-printing-mode preserves 1.13 float & complex str #10034

Merged

ahaldane force-pushed the legacy_mode_fixes branch from 0e2e17b to e2be765 Compare November 15, 2017 23:27

eric-wieser reviewed Nov 16, 2017

View reviewed changes

ahaldane force-pushed the legacy_mode_fixes branch from e2be765 to 1c77282 Compare November 16, 2017 18:55

ahaldane changed the title ~~MAINT: Legacy mode fixes~~ MAINT: Legacy mode specified as string, fix all-zeros legacy bug Nov 16, 2017

eric-wieser reviewed Nov 17, 2017

View reviewed changes

ahaldane added 2 commits November 17, 2017 16:20

MAINT: make legacy option support future legacy modes

dd31f13

Fixes numpy#10020

BUG: float arrays with no non-zero elements break legacy mode

abfddda

Fixes numpy#10026

ahaldane force-pushed the legacy_mode_fixes branch from 1c77282 to 8eebda7 Compare November 17, 2017 21:22

ahaldane commented Nov 17, 2017

View reviewed changes

MAINT: cleanup FloatingFormat code

3416cc8

ahaldane force-pushed the legacy_mode_fixes branch from 8eebda7 to 3416cc8 Compare November 17, 2017 21:37

charris reviewed Nov 18, 2017

View reviewed changes

eric-wieser reviewed Nov 18, 2017

View reviewed changes

eric-wieser approved these changes Nov 18, 2017

View reviewed changes

ahaldane force-pushed the legacy_mode_fixes branch from c371c08 to 86bd257 Compare November 18, 2017 02:59

MAINT: Add .. versionadded :: to parameter documentation.

dfc1164

Note that the `legacy` keyword was added in NumPy 1.14.0. [ci skip]

ahaldane force-pushed the legacy_mode_fixes branch 3 times, most recently from 3b3cde8 to 85dabca Compare November 18, 2017 03:25

MAINT: add back in errstate ignore in FloatingFormat

47a12f1

ahaldane force-pushed the legacy_mode_fixes branch from 85dabca to 47a12f1 Compare November 18, 2017 03:53

charris merged commit e5f4ac0 into numpy:master Nov 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Legacy mode specified as string, fix all-zeros legacy bug #10030

MAINT: Legacy mode specified as string, fix all-zeros legacy bug #10030

ahaldane commented Nov 15, 2017 •

edited

Loading

eric-wieser Nov 16, 2017

charris Nov 16, 2017

eric-wieser commented Nov 16, 2017

lesteve commented Nov 16, 2017

charris commented Nov 16, 2017

ahaldane commented Nov 16, 2017 •

edited

Loading

eric-wieser Nov 17, 2017 •

edited

Loading

eric-wieser Nov 17, 2017

charris Nov 18, 2017

eric-wieser Nov 18, 2017

ahaldane commented Nov 17, 2017

ahaldane Nov 17, 2017

charris Nov 18, 2017

eric-wieser Nov 18, 2017

eric-wieser Nov 18, 2017

eric-wieser Nov 18, 2017

charris Nov 18, 2017

eric-wieser Nov 18, 2017

ahaldane Nov 18, 2017

charris Nov 18, 2017

charris Nov 18, 2017

eric-wieser Nov 18, 2017

eric-wieser commented Nov 18, 2017

eric-wieser left a comment •

edited

Loading

eric-wieser Nov 18, 2017

ahaldane Nov 18, 2017

charris commented Nov 18, 2017

charris commented Nov 18, 2017

MAINT: Legacy mode specified as string, fix all-zeros legacy bug #10030

MAINT: Legacy mode specified as string, fix all-zeros legacy bug #10030

Conversation

ahaldane commented Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Nov 16, 2017

lesteve commented Nov 16, 2017

charris commented Nov 16, 2017

ahaldane commented Nov 16, 2017 • edited Loading

eric-wieser Nov 17, 2017 • edited Loading

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

ahaldane commented Nov 17, 2017

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

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

eric-wieser commented Nov 18, 2017

eric-wieser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Nov 18, 2017

charris commented Nov 18, 2017

ahaldane commented Nov 15, 2017 •

edited

Loading

ahaldane commented Nov 16, 2017 •

edited

Loading

eric-wieser Nov 17, 2017 •

edited

Loading

eric-wieser left a comment •

edited

Loading