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

ENH: Allow usage of custom library to serialize with to_json method. #54516

Closed
wants to merge 4 commits into from
Closed

ENH: Allow usage of custom library to serialize with to_json method. #54516

wants to merge 4 commits into from

Conversation

rmhowe425
Copy link
Contributor

@jbrockmendel
Copy link
Member

My intuition is that passing a custom serializer is going to break a ton of stuff. e.g. would you expect the stdlib json.dumps to work in the existing tests? im pretty sure it wouldnt

@rmhowe425
Copy link
Contributor Author

rmhowe425 commented Aug 14, 2023

My intuition is that passing a custom serializer is going to break a ton of stuff. e.g. would you expect the stdlib json.dumps to work in the existing tests? im pretty sure it wouldnt

Yeah adding functionality to add json.dumps did break a lot of tests. That’s why I also added an additional ‘engine_kwargs’ parameter so that I could pass the appropriate parameters for the custom serializer.

A lot of the I/O methods already support ‘engine_kwargs’ so adding this would follow API standardization.

@jbrockmendel what do you think?

@jbrockmendel
Copy link
Member

What is the actual use case here? Do you have a serializer that is more performant? Are there dtypes/scalars that are currently unsupported that you want to be supported?

@mroeschke
Copy link
Member

Yeah I don't think completely sidestepping the existing implementation isn't nicely maintainable here.

@rmhowe425
Copy link
Contributor Author

@mroeschke Do we want to close this PR and the corresponding issue then?

It looks like the Needs Triage label was removed for the issue roughly 2 years ago. It sounds like the acceptance criteria for the issue is no longer needed, or needs to be updated?

@mroeschke
Copy link
Member

Yeah let's close this issue pending further discussion. Usually any enhancement request requires more buy in from the core team before a PR

@mroeschke mroeschke closed this Aug 16, 2023
@rmhowe425 rmhowe425 deleted the dev/json/custom-arg branch February 17, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Allow usage of custom library to serialize with to_json method.
3 participants