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

EA: require size instead of __len__ #28389

Closed
wants to merge 5 commits into from

Conversation

jbrockmendel
Copy link
Member

Discussed here

Change the EA interface to require size instead of __len__.

Doing this separately from #27142 makes it much simpler to explain to downstream authors what is changing for them. After this, the rest of the changes necessary for (limited) 2D support should not affect downstream authors.

@jbrockmendel
Copy link
Member Author

looks like the docs thing is driven by a URLError

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.

do we need tests to guarantee this method exists in EA? e.g. for decmial / json / arrow bool does it?

@@ -40,6 +40,7 @@ Other enhancements
Backwards incompatible API changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

- The :class:`ExtensionArray` interface now requires the author to implement ``size`` instead of ``__len__``. The ``size`` method must _not_ depend on either ``__len__`` or ``shape`` (:issue:`????`)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use this issue number

@jreback jreback added the ExtensionArray Extending pandas with custom dtypes or arrays. label Sep 13, 2019
@jbrockmendel
Copy link
Member Author

do we need tests to guarantee this method exists in EA? e.g. for decmial / json / arrow bool does it?

This PR updates all of our internal (including tests.extension) EAs appropriately, and this is indirectly tested.

I've been giving some thought to how to test the "size must not depend on shape or len" requirement. Thinking of something along the lines of a test that monkeypatches shape and __len__ to raise, then calls size. That leaves a corner case where an author could write a size property that only calls shape or __len__ in some cases but not others. Not sure this is worth worrying about. Thoughts?

@TomAugspurger
Copy link
Contributor

Can we schedule a call sometime this week to talk through this?

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Sep 16, 2019

Can we schedule a call sometime this week to talk through this?

Yes that is a good idea I think.

I am -1 on this PR until there is a clear vision and agreement on the next steps.

@jbrockmendel
Copy link
Member Author

Wednesday or Thursday after 9 AM Pacific Time work for me

@jorisvandenbossche
Copy link
Member

Both work for me (although I would prefer one hour earlier or 1.5 hour later)

@jreback
Copy link
Contributor

jreback commented Sep 17, 2019

these times are ok

@jbrockmendel
Copy link
Member Author

An hour earlier on Wednesday works for me. Pending Tom's schedule, let's go for Wednesday at 15:00 UTC.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 17, 2019 via email

@jreback
Copy link
Contributor

jreback commented Sep 18, 2019 via email

@TomAugspurger
Copy link
Contributor

I think 15:00 UTC / 11:00 Eastern / 8:00 Pacific.

@TomAugspurger
Copy link
Contributor

cc @pandas-dev/pandas-core we're having a call on this in ~25 minutes.

https://meet.google.com/gvh-okpg-qux

https://docs.google.com/document/d/1k_E_1oSV9VNHgGzepdeCyFdju8ZaXDwI3CvDYkhOtQ8/edit?usp=sharing

@simonjayhawkins
Copy link
Member

simonjayhawkins commented Sep 18, 2019 via email

@jbrockmendel
Copy link
Member Author

Closing to clear the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExtensionArray Extending pandas with custom dtypes or arrays.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants