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.

Sign up for GitHub

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

Jump to bottom

Fix errors when trying to run ecephys write_nwb with LIMS #1503

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Fix errors when trying to run ecephys write_nwb with LIMS #1503

merged 2 commits into from
Apr 24, 2020

Conversation

njmei
Copy link
Contributor

@njmei njmei commented Apr 16, 2020 •
edited
Loading

This is a combination of 2 commits that resolve bugs that were preventing the ecephys write_nwb module from running to completion.

@njmei
Fix schema error when running ecephys write_nwb with LIMS
40d59f1 
LIMS calls the write_nwb module and specifies ecephys channel
structure id and acronym as `manual_structure_id` and
`manual_structure_acronym` in the input json.

It appears #1075 modified the ecephys write_nwb schema
from the expected values.

This commit changes back the ecephys write_nwb schema to expect:
`manual_structure_id` and `manual_structure_acronym`.
@njmei njmei requested review from NileGraddis and kschelonka April 16, 2020 22:49
@njmei njmei requested a review from sgratiy as a code owner April 16, 2020 22:49
@wbwakeman wbwakeman added the braintv relates to Insitute BrainTV program label Apr 22, 2020
@njmei njmei changed the title Fix schema error when running ecephys write_nwb with LIMS Fix errors when trying to run ecephys write_nwb with LIMS Apr 23, 2020
@njmei
Fix and refactor ecephys nwb spike filter and sorting
614e0ab 
Previously (#1365), filtering and sorting of ecephys spike
data was implemented for ecephys data. That PR added
filtering/sorting of spike data when writing and loading
from ecephys nwbfiles. It turns out the filtering/sorting
implementation on the nwb writing side was assuming
data in the units table that had yet to be added,
resulting in errors when trying to write ecephys
nwb files.

This commit 1) fixes the spike filtering and sorting
functionality 2) Removes filtering/sorting when loading
from ecephys nwbfiles 3) applies some refactoring to more
easily test adding probe data (e.g. unit tables)
to the ecephys nwbfiles and 4) adds tests.

Relates to: #1510
@njmei njmei force-pushed the fix-write-nwb-schema branch from b5e89e8 to 614e0ab Compare April 23, 2020 18:07
@njmei njmei requested a review from jsiegle April 23, 2020 18:08
@njmei
Copy link
Contributor Author

njmei commented Apr 23, 2020

@NileGraddis I noticed that there is a mean_waveforms dictionary in addition to spike_times and spike_amplitudes should mean_waveforms also go through the same filter/sorting process that spike_times and spike_amplitudes do?

@njmei njmei mentioned this pull request Apr 23, 2020
Closed
2 tasks
@jsiegle
Copy link
Collaborator

jsiegle commented Apr 24, 2020

I think this looks good! Have you looked at NWB files created before/after these changes to make sure they're the same?

@njmei @NileGraddis the mean_waveforms dict does not have to be updated in the same way; only spike_times and spike_amplitudes contain arrays of size N = total number of spikes per unit.

@njmei
Copy link
Contributor Author

njmei commented Apr 24, 2020 •
edited
Loading

@jsiegle Thanks for the review! These fixes are being merged into the feature/update-pynwb-and-hdmf branch. Because of pynwb/hdmf differences between the version we previously pinned (1.0.1) and what this branch allows (>=1.0.1, <2.0.0), I would expect there to be some differences between the previously released NWB files and the NWB files that this branch produces.

There are additional changes to how ecephys data is stored in NWB files that still need to be made (#1419, #1421, #1422, #1423, #1482). Once that work is complete, I'm planning on running through all the neuropixels examples notebooks with new NWB files produced by this branch to make sure everything is working properly.

Let me know if there are any other questions or concerns!

@jsiegle
Copy link
Collaborator

jsiegle commented Apr 24, 2020

That sounds good! I'd love to take a look at the new NWBs once they are ready.

NileGraddis
NileGraddis approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@NileGraddis NileGraddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice job on the probewise data test

@njmei njmei merged commit 1c1e406 into feature/update-pynwb-and-hdmf Apr 24, 2020
@kschelonka kschelonka deleted the fix-write-nwb-schema branch October 2, 2020 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NileGraddis NileGraddis NileGraddis approved these changes

@jsiegle jsiegle jsiegle approved these changes

@kschelonka kschelonka Awaiting requested review from kschelonka

@sgratiy sgratiy Awaiting requested review from sgratiy

Assignees
No one assigned
Labels
braintv relates to Insitute BrainTV program
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@njmei @jsiegle @NileGraddis @wbwakeman