-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
WIP: EA SetArray #22382
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
WIP: EA SetArray #22382
Conversation
Hello @h-vetinari! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 16, 2018 at 06:32 Hours UTC |
@h-vetinari Thanks a lot for working on this! To repeat my comment of the other PR, I personally don't think this should be necessarily included in pandas core, but would be perfect as an external package (now we have the EA interface). We have a dummy JSONArray implementation in the test suite that might run into similar problems, but for now that was not given much attention.
The
Can you give a more concrete example to show what you mean?
The things you mention above (except the last one) are mentioned in the docstrings in pandas/tests/extensions/conftest.py ? But please feel free to add any better documentation on things that are unclear! |
@@ -1482,6 +1484,45 @@ def _bool_method_SERIES(cls, op, special): | |||
code duplication. | |||
""" | |||
|
|||
def dispatch_to_extension_op(op, left, right): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a repetition of the existing dispatch function?
I had edited the OP a few minutes before you responded, because I had finally found I'll respond to the rest later, no time right now. |
closing as stale. would be more amenable as an EA. |
This is very WIP, but might IMO be helpful in figuring out the EA interfaces, since it has some other requirements than the EAs so far. It makes some way towards #4480 and might be a basis for #21547, where the comments were effectively "make this an EA", e.g. @jreback:
The code works so far, and tests pass, but there's a large amount of issues (and probably some conflicts with some other current EA-PRs). As a disclaimer, in each case it may well be that I'm misunderstanding something, but I've tried my best, and the same issues would probably be encountered by other EA authors.
fillna
andastype
do not dispatch to the EA impldf.fillna
not dispatching to Seriesset
is a case, where the nan-value is incompatible with the dtype in normal operations (i.e.{1} | np.nan
raises). This means that tests likeTestMethods.test_combine_add
andTestMethods.test_combine_le
will always fail on missing dataop(EA, np.ndarray)
is the same asop(np.ndarray, EA)
, or if the latter downcasts EA.data[-1
must be non-na forBaseGetitemTests.test_take
tests/extension/conftest.py
too late, will need to adapt my fixturespandas/tests/extension/base/ops.py
will cause friction, but those are just WIP as well. In any case,tests
likeBaseComparisonOpsTests.test_compare_array
can never suceed with comparing0
to a non-numeric dtype (and I wrapped it into aSeries
since I was getting errors with the dispatch as a list object)If/when this is ever merged, I'd like add more options to
.astype('set')
(example in #4480), and add a set accessor for other methods (again #4480).