-
Notifications
You must be signed in to change notification settings - Fork 22
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
Pickle memory maps in SliceableDataChunkIterator #536
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
" use DataInterface.add_to_nwbfile()." | ||
" use DataInterface.add_to_nwbfile().", | ||
stacklevel=2, |
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.
Found this useful in a separate conversion project; kept getting the warning but was unsure where it was coming from (had several data interfaces in the conversion)
src/neuroconv/tools/hdmf.py
Outdated
from hdmf.data_utils import GenericDataChunkIterator as HDMFGenericDataChunkIterator | ||
|
||
# from hdmf.data_utils import GenericDataChunkIterator as HDMFGenericDataChunkIterator | ||
from hdmf.data_utils import GenericDataChunkIterator | ||
|
||
class GenericDataChunkIterator(HDMFGenericDataChunkIterator): | ||
|
||
# class GenericDataChunkIterator(HDMFGenericDataChunkIterator): | ||
class _GenericDataChunkIterator(GenericDataChunkIterator): |
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.
This was a temporary reversion, I think I can restore it - just need to test it doesn't break the serialization
@@ -90,3 +95,42 @@ def _get_maxshape(self) -> tuple: | |||
|
|||
def _get_data(self, selection: Tuple[slice]) -> np.ndarray: | |||
return self.data[selection] | |||
|
|||
def is_pickleable(self) -> bool: | |||
if isinstance(self.data, (np.memmap, np.array)): |
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.
Will need to remove np.array
here; easy support is only offered for memory-map like lazy access to source data
if np.issubdtype(extended_data.dtype, np.object_): | ||
extended_data = extended_data.astype("str", copy=False) | ||
# if np.issubdtype(extended_data.dtype, np.object_): | ||
# extended_data = extended_data.astype("str", copy=False) |
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.
TODO: revert
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #536 +/- ##
==========================================
- Coverage 91.59% 91.06% -0.54%
==========================================
Files 96 96
Lines 4806 4833 +27
==========================================
- Hits 4402 4401 -1
- Misses 404 432 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sister PR to hdmf-dev/hdmf#924 and hdmf-dev/hdmf-zarr#111
This acts as a first test implementation by allowing efficient pickling of a memory-map based iterator. Will add and test more conditions over time
Also, to be clear, a
np.memmap
(and Zarr arrays for that matter) are pickleable by default; however, they do it by pickling the data they contain, not a packaged set of instructions for re-instantiating that object after reloading (in our case, on a separate process)So we want to override such methods to more efficiently allow re-initialization of the lazy mappings to the source data
Opening in draft for now