-
Notifications
You must be signed in to change notification settings - Fork 248
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
NWB Inspector Tests #1121
base: master
Are you sure you want to change the base?
NWB Inspector Tests #1121
Conversation
Hello @legouee! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-07-01 13:20:08 UTC |
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.
It seems like the mapping between the neo nwb structure is still developing. Maybe it would be good to introduce a mapping version that facilitates the correct reading of the nwb file?
neo/io/nwbio.py
Outdated
nwbfile.add_epoch_column('_name', 'the name attribute of the Epoch') | ||
# nwbfile.add_epoch_column('_description', 'the description attribute of the Epoch') | ||
nwbfile.add_epoch_column( | ||
'segment', 'the name of the Neo Segment to which the Epoch belongs') | ||
nwbfile.add_epoch_column('block', | ||
'the name of the Neo Block to which the Epoch belongs') | ||
nwbfile.add_trial_column('segment', 'name of the Segment to which the Epoch belongs') | ||
nwbfile.add_trial_column('block', 'name of the Block to which the Epoch belongs') |
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.
if nwb is supporting the epoch
concept, what is the reason to switch to trials
now?
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.
Yes. You are right. As it supports the epoch concept, it is not necessary to have the trials function.
I put it back as before.
nwbfile.add_acquisition(tS_evt) | ||
return tS_evt | ||
|
||
def _write_epoch(self, nwbfile, epoch): | ||
def _write_manage_epoch(self, nwbfile, segment, epoch, arr): |
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 method could benefit from having a docstring. What exactly is arr
here?
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.
arr is an epoch array with t_start and t_stop sorted in ascending order.
I don't think it's necessary to create a docstring for this function. It's just an intermediate step in order to respect NWBInspector.
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.
If you sort t_start and t_stop independently, doesn't this corrupt data in case one epoch duration is contained in another?
): | ||
for j in [label]: | ||
t_stop = t_start + duration | ||
seg_name = "%s %s" % (epoch.segment.name, label) |
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.
Here occurs the same error as with the events earlier. You have to keep a reference to the segment from before the proxy object is loaded.
NWB Inspector is a new tool to help inspect NWB files for compliance with NWB Best Practices :
https://github.com/NeurodataWithoutBorders/nwbinspector
The following pull request allows the validation of files created with Neo with respect to NWB Inspector.