WIP: support .nwb.lindi.json files #947
Conversation
|
See this relevant discussion... monkey patching h5py may or may not be the right approach. |
magland
changed the title
|
Based on NeurodataWithoutBorders/lindi#58 |
|
Updated the PR to use a new class H5pyFile in all the of the pynwb.NWBHDF5IO calls. This is equal to lindi.File in the case that lindi can be imported and is equal to h5py.File otherwise. lindi.File is the same as h5py.File except when the file path ends with .lindi.json. |
| # end with ".lindi.json", then H5pyFile is equivalent to h5py.File. Thus, the | ||
| # difference only matters when opening LINDI files. | ||
|
|
||
| USE_LINDI = os.environ.get("SPYGLASS_LINDI", "0") == "1" |
There was a problem hiding this comment.
I'd like to move us away from env var in favor of putting everything in src/spyglass/setting.py, and grabbing from the custom field in dj_config
There was a problem hiding this comment.
Sounds good. Could you give me some hints about the best way to do this?
There was a problem hiding this comment.
I think I would add it as an item like test_mode at L145 and then again at 215. To fetch it from settings as a bare variable, it would also need a line around 516. For demo, we should also mirror the name in the example config.
| if USE_LINDI: | ||
| from lindi import File as H5pyFile # noqa: F401 | ||
| else: | ||
| from h5py import File as H5pyFile # noqa: F401 |
There was a problem hiding this comment.
It seems like there are quite a few places where you're using multiple contexts
with H5pyFile(analysis_nwb_file_abs_path, "a") as h5f:
with pynwb.NWBHDF5IO(
file=h5f,
mode="a",
load_namespaces=True,
) as io:Is it possible to define a context manager here that would clean up those instances by combining them into something like ...?
with H5pyNwb(
analysis_nwb_file_abs_path,
mode="a",
load_namespaces=True,
) as h5f:There was a problem hiding this comment.
Yeah I that would be possible, but I thought it's better to be explicit.
There was a problem hiding this comment.
Sure, I think I was part motivated by the readability of the diff, and part motivated by a desire to see spyglass have less nesting than it does. If it's sometimes just one or the other, we can keep them separate. If it's always both, I'd be in favor of combining
| @@ -102,8 +102,8 @@ def plot_all_dio_events(self, return_fig=False): | |||
| va="bottom", | |||
| ) | |||
| ax.step( | |||
| np.asarray(event["dio"].timestamps), | |||
| np.asarray(event["dio"].data), | |||
| event["dio"].timestamps[:], | |||
There was a problem hiding this comment.
LINDI provides a h5py-like object which serves as a drop-in replacement, but does not support all of the low-level functionalities. It seems that np.asarray(h5py_dataset) uses some of these low-level functions. The change I made is to be compatible with lindi.
Description
These are the changes needed to support .nwb.lindi.json files in addition to .nwb in spyglass.
Optimistically, no other LINDI-changes to spyglass will be needed because LINDI provides a patch to h5py that allows it to automatically handle .lindi.json files based on their extension.For more information about LINDI:
https://github.com/neurodataWithoutBorders/lindi
Checklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.