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

842 dump and load scalars of sessions #843

Conversation

alixdamman
Copy link
Collaborator

No description provided.

@alixdamman alixdamman requested a review from gdementen January 9, 2020 13:56
larray/core/session.py Outdated Show resolved Hide resolved
res = LGroup(key=key, name=name, axis=axis)
except KeyError:
scalar_key = key
attrs = store.root._v_attrs

This comment was marked as resolved.

Copy link
Contributor

@gdementen gdementen Jan 9, 2020

Choose a reason for hiding this comment

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

From the description of the "obj" argument of the tables.Array class (at http://www.pytables.org/usersguide/libref/homogenous_storage.html#tables.Array), it seems like Python scalars might be natively supported by pytables...

doc/source/changes/version_0_33.rst.inc Outdated Show resolved Hide resolved
doc/source/changes/version_0_33.rst.inc Outdated Show resolved Hide resolved
larray/core/session.py Outdated Show resolved Hide resolved
larray/core/session.py Outdated Show resolved Hide resolved
larray/inout/common.py Outdated Show resolved Hide resolved
larray/inout/hdf.py Outdated Show resolved Hide resolved
axis = read_hdf(filepath_or_buffer, attrs['axis_key'])
res = LGroup(key=key, name=name, axis=axis)
elif _type in {cls.__name__ for cls in _supported_scalars_types}:
res = pd_obj.values
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you should not finish #761 before this PR, because supporting scalars via Pandas is not actually needed. We will need to support them in the "direct"/#761 path anyway and have to support Arrays, Axis and Groups on both (for backward compatibility) be we can skip scalars via Pandas (that would mean a few lines of code fewer to maintain).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it is there, let's finish this. I don't know when I will go back to work on PR #761 (partly because I don't like to work with pytables and prefer h5py but my intuition tells me we need pytables for our case).

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought h5py had the same features than pytables with a different API, except for querying (and optionally indexing) tables. And since we wouldn't need either feature, I would think using h5py would not be a problem. That said I never used h5py so I don't know for sure.

In either case, I wouldn't use h5py anyway because pandas relies on pytables and since we will have to keep the current code (using pytables indirectly) for backward compat for a while, depending on both pytables and h5py would be weird.

larray/inout/hdf.py Outdated Show resolved Hide resolved
larray/inout/hdf.py Outdated Show resolved Hide resolved
larray/inout/hdf.py Outdated Show resolved Hide resolved
larray/inout/pickle.py Outdated Show resolved Hide resolved
larray/inout/pickle.py Outdated Show resolved Hide resolved
@alixdamman

This comment has been minimized.

@gdementen

This comment has been minimized.

larray/inout/common.py Outdated Show resolved Hide resolved
@alixdamman
Copy link
Collaborator Author

OK with the last commit?

@gdementen
Copy link
Contributor

sure

@alixdamman
Copy link
Collaborator Author

So, OK to merge?

Copy link
Contributor

@gdementen gdementen left a comment

Choose a reason for hiding this comment

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

yes, LGTM

@alixdamman alixdamman force-pushed the 842_dump_and_load_scalars_of_sessions branch from 1f4f6db to d8a1561 Compare January 22, 2020 13:23
@alixdamman
Copy link
Collaborator Author

Thanks for the review.

@alixdamman alixdamman merged commit e6239d0 into larray-project:master Jan 22, 2020
@alixdamman alixdamman deleted the 842_dump_and_load_scalars_of_sessions branch January 22, 2020 13:27
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.

2 participants