-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
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! |
| 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.
fillnaandastypedo not dispatch to the EA impldf.fillnanot dispatching to Seriessetis a case, where the nan-value is incompatible with the dtype in normal operations (i.e.{1} | np.nanraises). This means that tests likeTestMethods.test_combine_addandTestMethods.test_combine_lewill always fail on missing dataop(EA, np.ndarray)is the same asop(np.ndarray, EA), or if the latter downcasts EA.data[-1must be non-na forBaseGetitemTests.test_taketests/extension/conftest.pytoo late, will need to adapt my fixturespandas/tests/extension/base/ops.pywill cause friction, but those are just WIP as well. In any case,testslikeBaseComparisonOpsTests.test_compare_arraycan never suceed with comparing0to a non-numeric dtype (and I wrapped it into aSeriessince 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).