-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ds.to_dict with data as arrays, not lists
#7739
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
Conversation
|
I would appreciate any edification on the Mypy failures. Looking at the indicated lines, i'm 🤷 . |
|
I solved the mypy errors in a highly dubious way. 👀 |
|
In the off-hand chance this is reviewed before I push again, do not merge. I have a fix to encodings not getting properly roundtripped in Ds.from_dict(ds.to_dict). it was minor to fix but making sure it's tested will take a min |
|
@dcherian thanks! I didnt incoroprate any suggestions yet. |
|
i kinda implied, but I'll just state that the extra code to test equality of encodings is not handsome. |
|
I'm happy to "fix" the mypy issues, but it's on that I suspect might be requested for changes (if I recall correctly, it's just in the tests) |
|
Copying my comment from #1599 (comment)
|
ds.to_dict with data as arrays, not lists
headtr1ck
left a comment
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.
Looks good already.
Just a couple of minor comments.
And a design question/suggestion: what about instead of adding another kwarg, you could use data = True / False / "numpy"?
|
Making all the requested changes, the above should resolve momentarily. I like this "trick"/suggestion: I will implement this if we are in agreement with @dcherian |
Oh yeah, I like this. Only suggestion is |
…of Python datatypes, "array" returns numpy.ndarrays, False returns only the schema
|
I followed |
|
Thanks @jmccreight great work! |
whats-new.rstapi.rst