-
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 methods to find objects of a given neurodata type / pynwb class #1737
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #1737 +/- ##
==========================================
- Coverage 91.99% 91.82% -0.17%
==========================================
Files 27 27
Lines 2623 2630 +7
Branches 685 688 +3
==========================================
+ Hits 2413 2415 +2
- Misses 138 143 +5
Partials 72 72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/pynwb/__init__.py
Outdated
read_nwbfile = self.read() | ||
return read_nwbfile.find_all_of_class(pynwb_cls) |
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.
Could you clarify what use-case this function addresses that NWBFile.find_all_of_class
does not?
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 added a comment in the docstring. It says: This method is useful for getting neurodata type objects from cached extensions where you do not have easy access to a python class to pass to NWBFile.find_all_of_class
.
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.
Makes sense. But it seems a bit strange that this functionality is in two different places. Also, with this we would need to duplicate this also for Zarr. Instead, I'm wondering whether find_all_of_class
can be modified to do both, something like:
@docval({'name': 'neurodata_types', 'type': (type, str), 'doc': 'The PyNWB container class to search for instances of or the string with the name of the neurodata_type'},)
def find_all(self, neurodata_type):
if isinstance(neurodata_type, str):
pynwb_cls = self.io.manager.type_map.get_dt_container_cls(neurodata_type, namespace)
else:
pynwb_cls = neurodata_type
ret = [obj for obj in self.objects.values() if isinstance(obj, pynwb_class)]
return ret
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.
Yeah, that works too. I'll update it
Idea LGTM, just needs a quick test or two |
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 is a nice idea. I have two suggestions:
- I'm not crazy about the name. I would prefer something a bit more explicit, like
find_all_of_type
- I would rather this live in
Container
or maybeNWBContainer
. That way it could be used on any container, not justNWBFile
.
And yes, I of course agree with @CodyCBakerPhD that this will need tests
I'm wondering whether it would be simpler from a user perspective to have this part of the existing Line 530 in 46e8878
|
Motivation
A common feature request is to find objects of a given neurodata type or pynwb class.
For example:
pynwb.file.Subject
Fix #560
TODO
Checklist
flake8
from the source directory.