-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add read_nwb_method
for local paths in both hdf5 and zarr
#1994
base: dev
Are you sure you want to change the base?
Add read_nwb_method
for local paths in both hdf5 and zarr
#1994
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1994 +/- ##
==========================================
- Coverage 91.69% 91.39% -0.30%
==========================================
Files 27 27
Lines 2708 2719 +11
Branches 707 710 +3
==========================================
+ Hits 2483 2485 +2
- Misses 149 158 +9
Partials 76 76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Sorry for the delay @h-mayorquin! Added my review comments
from hdmf_zarr import NWBZarrIO | ||
backend_is_zarr = NWBZarrIO.can_read(path=path) | ||
if backend_is_zarr: | ||
return NWBZarrIO.read_nwb(path=path) | ||
else: | ||
raise ValueError(f"Unsupported backend for file: {path}") |
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.
from hdmf_zarr import NWBZarrIO | |
backend_is_zarr = NWBZarrIO.can_read(path=path) | |
if backend_is_zarr: | |
return NWBZarrIO.read_nwb(path=path) | |
else: | |
raise ValueError(f"Unsupported backend for file: {path}") | |
try: | |
from hdmf_zarr import NWBZarrIO | |
backend_is_zarr = NWBZarrIO.can_read(path=path) | |
if backend_is_zarr: | |
return NWBZarrIO.read_nwb(path=path) | |
else: | |
raise ValueError(f"Unsupported backend for file: {path}") | |
except ImportError: | |
raise ValueError(f"Unsupported backend for file: '{path}'. If you are trying to read a Zarr file, make sure you have hdmf-zarr installed.") |
I believe read_nwb will currently return an ImportError if the user provides an invalid path but does not have hdmf_zarr installed. Can you support this case?
self.assertContainerEqual(read_nwbfile, self.nwbfile) | ||
read_nwbfile.get_read_io().close() | ||
|
||
@unittest.skipIf(not HAVE_NWBZarrIO, "NWBZarrIO library not available") |
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.
Can you add tests for
- the user providing a path that does not exist
- if the user does not have hdmf_zarr installed and provides an unsupported file path
This function uses the following defaults: | ||
* Always opens in read-only mode | ||
* Automatically loads namespaces | ||
* Detects file format based on extension |
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.
* Detects file format based on extension | |
* Detects file format based on whether supported backend IO classes can read the file |
I think in the current implementation other extensions will work since can_read
and IO.read_nwb
methods just check whether the IO can successfully open the file?
CHANGELOG.md
Outdated
- Removed `SpatialSeries.bounds` field that was not functional. This will be fixed in a future release. @rly [#1907](https://github.com/NeurodataWithoutBorders/pynwb/pull/1907), [#1996](https://github.com/NeurodataWithoutBorders/pynwb/pull/1996) | ||
- Added support for `NWBFile.was_generated_by` field. @stephprince [#1924](https://github.com/NeurodataWithoutBorders/pynwb/pull/1924) | ||
- Added support for `model_number`, `model_name`, and `serial_number` fields to `Device`. @stephprince [#1997](https://github.com/NeurodataWithoutBorders/pynwb/pull/1997) | ||
- Deprecated `EventWaveform` neurodata type. @rly [#1940](https://github.com/NeurodataWithoutBorders/pynwb/pull/1940) | ||
- Deprecated `ImageMaskSeries` neurodata type. @rly [#1941](https://github.com/NeurodataWithoutBorders/pynwb/pull/1941) |
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.
- Removed `SpatialSeries.bounds` field that was not functional. This will be fixed in a future release. @rly [#1907](https://github.com/NeurodataWithoutBorders/pynwb/pull/1907), [#1996](https://github.com/NeurodataWithoutBorders/pynwb/pull/1996) | |
- Added support for `NWBFile.was_generated_by` field. @stephprince [#1924](https://github.com/NeurodataWithoutBorders/pynwb/pull/1924) | |
- Added support for `model_number`, `model_name`, and `serial_number` fields to `Device`. @stephprince [#1997](https://github.com/NeurodataWithoutBorders/pynwb/pull/1997) | |
- Deprecated `EventWaveform` neurodata type. @rly [#1940](https://github.com/NeurodataWithoutBorders/pynwb/pull/1940) | |
- Deprecated `ImageMaskSeries` neurodata type. @rly [#1941](https://github.com/NeurodataWithoutBorders/pynwb/pull/1941) | |
- Removed `SpatialSeries.bounds` field that was not functional. This will be fixed in a future release. @rly [#1907](https://github.com/NeurodataWithoutBorders/pynwb/pull/1907), [#1996](https://github.com/NeurodataWithoutBorders/pynwb/pull/1996) | |
- Added support for `NWBFile.was_generated_by` field. @stephprince [#1924](https://github.com/NeurodataWithoutBorders/pynwb/pull/1924) | |
- Added support for `model_number`, `model_name`, and `serial_number` fields to `Device`. @stephprince [#1997](https://github.com/NeurodataWithoutBorders/pynwb/pull/1997) | |
- Deprecated `EventWaveform` neurodata type. @rly [#1940](https://github.com/NeurodataWithoutBorders/pynwb/pull/1940) | |
- Deprecated `ImageMaskSeries` neurodata type. @rly [#1941](https://github.com/NeurodataWithoutBorders/pynwb/pull/1941) |
These updates were indented to indicate schema 2.8.0 related changes
from pynwb import read_nwb | ||
|
||
nwbfile = read_nwb(filepath) |
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.
I think once the simplified read_nwb with streaming is supported we may want to go through the other tutorials that use io.read() in read only mode and update those as well.
Motivation
See #1974
How to test the behavior?
I added tests.
Checklist
ruff check . && codespell
from the source directory.