Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEPR: Categorical.ravel, get_dtype_counts, dtype_str, to_dense #29900

Merged
merged 11 commits into from Dec 1, 2019

Conversation

jbrockmendel
Copy link
Member

Starting in on the 0.25.0 section in #6581.

@jreback jreback added the Deprecate Functionality to remove in pandas label Nov 27, 2019
@jreback jreback added this to the 1.0 milestone Nov 27, 2019
@jreback
Copy link
Contributor

jreback commented Nov 27, 2019

sure

@jbrockmendel
Copy link
Member Author

@datapythonista im having trouble telling what the checks failure is about. one thing suggests it is in asvs, but another suggests annotations

@datapythonista
Copy link
Member

I just see this error in the benckmarks: https://github.com/pandas-dev/pandas/pull/29900/checks?check_run_id=323732409#step:13:4913

I guess it means that one of the 3 runs was too slow, but you'll know better.

It took me forever to find the error. I think it shouldn't be difficult to highlight failures in the benchmarks, so it's immediate to know what failed. I'll see if I can open a PR for that.

@jbrockmendel
Copy link
Member Author

Thanks for taking a look. I agree itd be nice to have an easier way to find when there is a broken asv. I'm hopeful that #29906 will fix this particular problem

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question / comment but other changes lgtm

stacklevel=2,
)
return np.array(self)
return self.view()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this change - was there a consensus on this being the return value or does it need discussion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in #27199

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine, though original we were returning self (and not a copy)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not a copy, im not sure if it matters if we return self vs self.view

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a shallow copy (a new object)

   def view(self, dtype=None):
        if dtype is not None:
            raise NotImplementedError(dtype)
        return self._constructor(values=self._codes, dtype=self.dtype, fastpath=True)

but suppose its ok, can you validate that it is a new object in the tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you validate that it is a new object in the tests?

After adding this to the test_ravel in the extensions tests, it looks like all the other EAs we test return self (because that is was the base class does so ill change this to just use the base class implementation and match the others.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a rebase and question.

@WillAyd WillAyd merged commit 9333e3d into pandas-dev:master Dec 1, 2019
@WillAyd
Copy link
Member

WillAyd commented Dec 1, 2019

Thanks @jbrockmendel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants