Skip to content
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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

legouee
Copy link
Contributor

@legouee legouee commented May 19, 2022

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.

@legouee legouee requested a review from apdavison May 19, 2022 11:57
@pep8speaks
Copy link

pep8speaks commented May 19, 2022

Hello @legouee! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 462:25: E126 continuation line over-indented for hanging indent
Line 467:25: E123 closing bracket does not match indentation of opening bracket's line
Line 727:50: W291 trailing whitespace
Line 728:48: W291 trailing whitespace
Line 729:42: W291 trailing whitespace

Comment last updated at 2022-07-01 13:20:08 UTC

Copy link
Member

@JuliaSprenger JuliaSprenger left a 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 Show resolved Hide resolved
neo/io/nwbio.py Outdated Show resolved Hide resolved
neo/io/nwbio.py Show resolved Hide resolved
neo/io/nwbio.py Outdated
Comment on lines 461 to 480
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')
Copy link
Member

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?

Copy link
Contributor Author

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.

neo/io/nwbio.py Outdated Show resolved Hide resolved
nwbfile.add_acquisition(tS_evt)
return tS_evt

def _write_epoch(self, nwbfile, epoch):
def _write_manage_epoch(self, nwbfile, segment, epoch, arr):
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

@JuliaSprenger JuliaSprenger added this to the 0.11.0 milestone Jun 23, 2022
neo/io/nwbio.py Outdated Show resolved Hide resolved
):
for j in [label]:
t_stop = t_start + duration
seg_name = "%s %s" % (epoch.segment.name, label)
Copy link
Member

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.

@apdavison apdavison modified the milestones: 0.10.3, 0.11.0 Aug 30, 2022
@apdavison apdavison modified the milestones: 0.11.0, 0.12.0 Sep 29, 2022
@JuliaSprenger JuliaSprenger modified the milestones: 0.12.0, 0.12.1 Apr 2, 2023
@JuliaSprenger JuliaSprenger modified the milestones: 0.12.0, 0.12.1 Apr 2, 2023
@JuliaSprenger JuliaSprenger modified the milestones: 0.12.1, 0.13.0 Jul 19, 2023
@apdavison apdavison modified the milestones: 0.13.0, 0.14.0, future Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants