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

REF: move values_for_json to EA #53680

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

@@ -1443,6 +1443,10 @@ def _reduce(self, name: str, *, skipna: bool = True, **kwargs):
# Non-Optimized Default Methods; in the case of the private methods here,
# these are not guaranteed to be stable across pandas versions.

def _values_for_json(self) -> np.ndarray:
# TODO: document!
Copy link
Member

Choose a reason for hiding this comment

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

Yeah would be good to have this before merging

Copy link
Member Author

Choose a reason for hiding this comment

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

@WillAyd thoughts on what would be useful here?

Copy link
Member

Choose a reason for hiding this comment

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

Guessing this should just be an object dtype array where any value is JSON serializable (ex: string, number, list, dict)

Copy link
Member Author

Choose a reason for hiding this comment

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

the status quo returns non-object ndarrays in many cases

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yea but that is also because the JSON serializer has extra logic to handle primitives for those types (eg: int64 backing for datetime). Is the idea here that general EAs need to define this or is this scoped to the internally managed EAs?

Copy link
Member Author

Choose a reason for hiding this comment

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

The real motivation is to try to move EA-specific logic from Blocks to EAs. I'm kind of hoping that we can stubmle onto a solution to #31917 along the way

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. For that issue @lithomas1 might be interested in implementing the serializer.

As far as this PR is concerned I am ok with it either way. Generally not sure it should end up in the EA in the long run, but doesn't hurt to move around today either

@mroeschke mroeschke added IO JSON read_json, to_json, json_normalize ExtensionArray Extending pandas with custom dtypes or arrays. labels Jun 20, 2023
@jbrockmendel
Copy link
Member Author

Added docstring. Not confident enough in how this works to add tests yet

@jbrockmendel
Copy link
Member Author

@WillAyd OK here?

@WillAyd
Copy link
Member

WillAyd commented Jul 10, 2023

I don't know that I see the vision for how this will actually work with JSON arguments (i.e. date formatting) but no objection to it either

@jbrockmendel
Copy link
Member Author

Unless there are more comments, I'm planning to merge later this week.

@mroeschke mroeschke added this to the 2.1 milestone Jul 25, 2023
@mroeschke mroeschke merged commit b379f99 into pandas-dev:main Jul 25, 2023
@mroeschke
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the ref-values_for_json branch July 25, 2023 16:56
jamie-harness pushed a commit to jamie-harness/pandas1 that referenced this pull request Jul 26, 2023
* REF: move values_for_json to EA

* docstring
jamie-harness added a commit to jamie-harness/pandas1 that referenced this pull request Jul 26, 2023
* REF: move values_for_json to EA

* docstring

Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
jamie-harness pushed a commit to jamie-harness/pandas1 that referenced this pull request Jul 26, 2023
* REF: move values_for_json to EA

* docstring
jamie-harness added a commit to jamie-harness/pandas1 that referenced this pull request Jul 26, 2023
* REF: move values_for_json to EA

* docstring

Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
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. IO JSON read_json, to_json, json_normalize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants