Categorical refactor #10212

tacaswell · 2018-01-10T02:42:32Z

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer · 2018-01-10T03:10:40Z

lib/matplotlib/category.py

@@ -1,128 +1,109 @@
-# -*- coding: utf-8 OA-*-za
+# -*- coding: utf-8 OA -*-


what's "OA"?
actually not even sure why this file needs an encoding cookie

anntzer · 2018-01-10T03:10:54Z

lib/matplotlib/category.py

-            return vmap[value]
-
-        vals = shim_array(value)
+        """Use axis.units mapping tncode data as floats."""


anntzer · 2018-01-10T03:14:33Z

lib/matplotlib/category.py

+                raise TypeError("{val!r} is not a string".format(val=k))
+            self._mapping[k] = int(v)
+        if self._mapping:
+            start = max(self._mapping.values()) + 1


I feel like if you pass an explicit mapping then either

you don't expect new categories to come in and you'd rather error out on new categories, or

you may want to have your own index-generator as well.

thoughts?

anntzer · 2018-01-10T03:15:48Z

lib/matplotlib/axis.py

@@ -750,15 +749,14 @@ def limit_range_for_scale(self, vmin, vmax):
        return self._scale.limit_range_for_scale(vmin, vmax, self.get_minpos())

    @property
+    @cbook.deprecated("2.1.1")


2.1.2, below the same

tacaswell · 2018-01-10T04:12:24Z

@anntzer 's comment about not letting further updates if the user provides an explicit initial mapping. I don't really see the down side of letting user expand on initial data. Once it is loaded and you have loaded the initial data up, it's not different than the case were after the first round of plotting we see some new categories.

story645 · 2018-01-10T04:26:38Z

I agree with @anntzer that I don't get why you'd let the user put in an initial mapping - or how they'd even do that. I agree that at that point they should just be doing it by hand, and it adds an I think unnecessary layer of complexity to the init.

story645 · 2018-01-10T04:34:12Z

lib/matplotlib/category.py


    @staticmethod
    def axisinfo(unit, axis):
-        majloc = StrCategoryLocator(axis.unit_data.locs)
-        majfmt = StrCategoryFormatter(axis.unit_data.seq)
+        majloc = StrCategoryLocator(axis.units)


It bugs me that locator and formatter are getting the mapping and there's not a separation, so at the least I think there should be a comment saying that this is because the Formatter and Locator require references rather than objects/copies.

story645 · 2018-01-10T04:36:44Z

lib/matplotlib/category.py

+
+class StrCategoryLocator(ticker.Locator):
+    def __init__(self, unit_data):
+        self._unit_data = unit_data


for consistency, shouldn't this be units?

story645 · 2018-01-10T04:44:49Z

lib/matplotlib/category.py

+    def __call__(self, x, pos=None):
+        if pos is None:
+            return ""
+        r_mapping = {v: k for k, v in self._unit_data._mapping.items()}


since the mapping is now an ordered dict starting at 0, isn't it really just an array where the index is the mapping? so basically list(self._unit_data.keys())[int(x)] ? (which makes me wonder if that shouldn't just be how the mapping is stored anyway....)

Or maybe the self._unit_data. should be a proper dict/hash like object? That has the added benefit of moving the getter function into the object implementation and then

if not isinstance(k, six.text_type): k = k.decode('utf-8') return axis.units._mapping[k]

can be in one place.

anntzer · 2018-01-15T22:17:00Z

The idea with user-provided mapping is that we don't even check that the converted values they provide are indeed consecutive integers starting with zero (and I'm not sure we should either), so (unless we do) incrementing a counter may or may not be a reasonable way to provide "new" converted values.

jklymak · 2018-01-19T19:14:01Z

lib/matplotlib/axes/_base.py

@@ -211,32 +211,24 @@ def set_patchprops(self, fill_poly, **kwargs):
        fill_poly.set(**kwargs)

    def _xy_from_xy(self, x, y):
+        def _check_numeric(inp):


This is raising all sorts of test errors for non-categorical...

phobson · 2018-01-30T17:09:47Z

doc/api/api_changes/2018-01_TAC.rst

+Simplify String Categorical handling
+------------------------------------
+
+ - Only handling `str` (not `bytes` in pyhon3).  This disallows using integers or


sp: python3

(sorry, I know we're supposed to push minor editorial changes directly, but I'm away from my main machine ATM)

- Expect that `axis.units` has a `_mapping` attribute. - Allow only strings. This prevents issues with mixed input where plotting different sub-sets of the data will or will not trigger the correct converter / unit behavior. - Do not allow missing data. Do not special case nan or inf for the same reasons as above. - deprecate `Axis.unit_data`. Axis already has a `units` attribute which hold a UnitData instance.

Change to using direct sub-classes of Locator and Formatter which hold a reference to the UnitData object. This makes the classes a bit less brittle and avoids having extra methods on the classes which will not work as intended.

Reattach the unitful data to Line2D.

This reverts commit aaefde4.

This reverts commit ec692d8.

tacaswell · 2018-02-05T12:50:09Z

The explicit for number post conversion was prematurely masking a different exception that we are testing for related to the shape so I just reverted that change.

The other changes to _base.py where somehow breaking pandas + datetime64, but reverting that seems to have broken some other things.

tacaswell · 2018-02-05T20:37:03Z

@tacaswell 's homework

rip out pre-defining the mapping
fix the tests
update the docs

jklymak · 2018-02-21T04:32:36Z

Should this still be open given that #9783 is merged?

dstansby · 2018-05-30T13:20:37Z

What's the status of this? I'm happy to put time into reviewing since it's labelled as release critical for 3.0, but just wanted to double check first!

story645 · 2018-05-30T18:51:11Z

Close for now because most of this went in via #9783 and much of what didn't is related to general unit inconsistencies and so should be a stand along PR anyway.

tacaswell added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: categorical labels Jan 10, 2018

tacaswell added this to the v2.2 milestone Jan 10, 2018

tacaswell requested a review from story645 January 10, 2018 02:42

anntzer reviewed Jan 10, 2018

View reviewed changes

tacaswell force-pushed the categorical_refactor branch 2 times, most recently from efe305e to 7d336f7 Compare January 10, 2018 04:07

story645 reviewed Jan 10, 2018

View reviewed changes

jklymak mentioned this pull request Jan 10, 2018

Don't convert numbers plotted on an axis with units #10206

Merged

jklymak reviewed Jan 19, 2018

View reviewed changes

jklymak mentioned this pull request Jan 22, 2018

Don't sort categorical keys. #9318

Closed

6 tasks

jklymak added the status: needs revision label Jan 24, 2018

phobson reviewed Jan 30, 2018

View reviewed changes

tacaswell force-pushed the categorical_refactor branch from 7d336f7 to e7664d6 Compare February 5, 2018 02:35

story645 and others added 10 commits February 4, 2018 21:47

MNT: start to re-factor the categorical implementation

4442304

MNT: remove typos and re-organize imports

98096e2

MNT: re-work how units are handled in axes._base

04edaa3

Reattach the unitful data to Line2D.

TST: fix up tests

283ca08

TST: add additional tests to categorical classes

6f73c24

WIP: add explicit test of conversion to float

ac355c8

WIP: fix broken error message

6457f4f

STY: whitespace clean up

3972c38

tacaswell and others added 11 commits February 4, 2018 21:47

DOC: document API changes

3c7b185

TST: add mixed-type failure tests

87bfe1e

added test for matplotlib#9843

d7a32f6

STY: remove utf marker

f68231d

DOC: improve docstring

78c5909

MNT: remove unused imports in category.py

c0e4851

TST: fix tests on py2

1c280f5

API: restore support for bytes

6beebcc

Revert "WIP: add explicit test of conversion to float"

7896cdc

This reverts commit aaefde4.

Revert "MNT: re-work how units are handled in axes._base"

6cc1841

This reverts commit ec692d8.

TST: make sure pandas tests actually draw

0ca66d2

tacaswell force-pushed the categorical_refactor branch from e7664d6 to 0ca66d2 Compare February 5, 2018 02:47

DOC: fix API docs

5515d3a

tacaswell modified the milestones: v2.2, v3.0 Feb 5, 2018

story645 added a commit to story645/matplotlib that referenced this pull request Feb 6, 2018

updated unitdata and locator/formatter against matplotlib#10212

5af7390

story645 mentioned this pull request Feb 6, 2018

Categorical: Unsorted, String only, fix overwrite bug #9783

Merged

story645 closed this May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Categorical refactor #10212

Categorical refactor #10212

tacaswell commented Jan 10, 2018

anntzer Jan 10, 2018 •

edited

Loading

anntzer Jan 10, 2018

anntzer Jan 10, 2018

anntzer Jan 10, 2018

tacaswell commented Jan 10, 2018

story645 commented Jan 10, 2018 •

edited

Loading

story645 Jan 10, 2018

story645 Jan 10, 2018

story645 Jan 10, 2018

story645 Jan 15, 2018 •

edited

Loading

anntzer commented Jan 15, 2018

jklymak Jan 19, 2018

phobson Jan 30, 2018

tacaswell commented Feb 5, 2018

tacaswell commented Feb 5, 2018

jklymak commented Feb 21, 2018

dstansby commented May 30, 2018

story645 commented May 30, 2018

		@@ -1,128 +1,109 @@
		# -- coding: utf-8 OA--za
		# -- coding: utf-8 OA --

Categorical refactor #10212

Categorical refactor #10212

Conversation

tacaswell commented Jan 10, 2018

PR Summary

PR Checklist

anntzer Jan 10, 2018 • 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

tacaswell commented Jan 10, 2018

story645 commented Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

story645 Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

anntzer commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Feb 5, 2018

tacaswell commented Feb 5, 2018

jklymak commented Feb 21, 2018

dstansby commented May 30, 2018

story645 commented May 30, 2018

anntzer Jan 10, 2018 •

edited

Loading

story645 commented Jan 10, 2018 •

edited

Loading

story645 Jan 15, 2018 •

edited

Loading