Deprecated fastpath parameter in Categorial constructor

[kaichogami]

Hello everyone!
I am new to Pandas developer community and your feedback would help me a lot. Thank you!

• closes API: Deprecate Categorical fastpath argument #17562

[pep8speaks]

Hello @kaichogami! Thanks for updating the PR.

• In the file pandas/core/categorical.py, following are the PEP8 issues :

Line 655:1: W293 blank line contains whitespace
Line 657:13: E303 too many blank lines (2)
Line 667:1: W293 blank line contains whitespace
Line 668:9: E303 too many blank lines (2)
Line 669:13: E129 visually indented line with same indent as next logical line

Comment last updated on November 13, 2017 at 12:27 Hours UTC

[kaichogami]

I am quite confused on what should be done for the fastpath parameters used in class method's to create a new Categorial. Should I replace them with dtype or let the __init__ handle CategorialDtype construction?

[kaichogami]

@TomAugspurger

[TomAugspurger]

Thanks @kaichogami!

I didn't quite understand what fastpath was doing earlier. It seems to only be valid when values is actually codes, so I think we should focus the effort on fixing from_codes (but the warning you put in place looks good and should stay).

Ok, so, we'll want to modify Categorical.from_codes to also take a dtype argument.

def from_codes(cls, codes, categories, ordered=None, dtype=None):
   ...

We'll do the validation of categories, ordered, and dtype, basically refactor the first section of __init__ to something like

def validate_dtype(values=None, categories=None, ordered=None, dtype=None):

        # Ways of specifying the dtype (prioritized ordered)
        # 1. dtype is a CategoricalDtype
        #    a.) with known categories, use dtype.categories
        #    b.) else with Categorical values, use values.dtype
        #    c.) else, infer from values
        #    d.) specifying dtype=CategoricalDtype and categories is an error
        # 2. dtype is a string 'category'
        #    a.) use categories, ordered
        #    b.) use values.dtype
        #    c.) infer from values
        # 3. dtype is None
        #    a.) use categories, ordered
        #    b.) use values.dtype
        #    c.) infer from values

        if dtype is not None:
            if isinstance(dtype, compat.string_types):
                if dtype == 'category':
                    dtype = CategoricalDtype(categories, ordered)
                else:
                    msg = "Unknown `dtype` {dtype}"
                    raise ValueError(msg.format(dtype=dtype))
            elif categories is not None or ordered is not None:
                raise ValueError("Cannot specify both `dtype` and `categories`"
                                 " or `ordered`.")

            categories = dtype.categories
            ordered = dtype.ordered

        elif is_categorical(values):
            dtype = values.dtype._from_categorical_dtype(values.dtype,
                                                         categories, ordered)
        else:
            dtype = CategoricalDtype(categories, ordered)
        return categories, ordered, dtype

And use that in both __init__ and from_codes.

Then, in from_codes, instead of return cls(codes, categories...), you'll do

self = cls.__new__(cls)
self._codes = coerce_indexer_dtype(codes, dtype.categories)
self._dtype = dtype
return self

I think that'll work. Then, anywhere you see a fastpath=True, you should be able to change it to use from_codes with a dtype, and it'll be as fast as possible. Make sense?

[kaichogami]

Yes, it does. I will go change it as soon as I can. Thanks for the feedback! 😄

[kaichogami]

Hi, I am a bit busy so if its something important somebody else can take it over, otherwise I will do it by next week. Thank you.

[TomAugspurger]

No rush on this.

[jreback]

can you rebase

[jreback]

closing as stale, if you want to work on this, pls ping.