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

Add new function to retrieve spatial series from NWB file #777

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

khl02007
Copy link
Collaborator

Description

Currently RawPosition is populated by loading spatial series with get_all_spatial_series, which calls get_data_interface(nwbfile, 'position', pynwb.behavior.Position), which looks for an object of name position and type pynwb.behavior.Position within the processing modules of the NWB file. Some NWB files (e.g. the Buzsaki lab data) don't use the name position (instead they use SubjectPosition). As a result, the spatial series in the NWB file don't show up in RawPosition upon ingestion into spygass.

This fixes this by creating a new function called get_position_obj which looks for object of the type pynwb.behavior.Position within the behavior processing module of the NWB file without caring about the name. get_all_spatial_series now calls this function instead of get_data_interface(nwbfile, 'position', pynwb.behavior.Position).

I have tested this on an NWB file from our lab and have confirmed that this doesn't cause any change in the output of get_all_spatial_series. I have also tested it on the Buzsaki lab NWB file and confirmed that this now returns the spatial series (used to return nothing).

Checklist:

  • This PR should be accompanied by a release: unsure
  • (If release) I have updated the CITATION.cff
  • I have updated the CHANGELOG.md
  • I have added/edited docs/notebooks to reflect the changes

@CBroz1
Copy link
Member

CBroz1 commented Jan 20, 2024

I think another approach would be to give the old get_data_interface a mapping, like a private dict, so we keep the logic centralized, rather than dev a new func for each data type when nwb is sometimes a moving target anyway

def central_func(params):
    lookup_dict = {'position': ['behavior', pynwb.behavior.position], 'other': ['field', type]}
    if param in lookup_dict:
        field, type = lookup_dict[param]
    else:
        field, type = default_field, other_default

But this works if getting the interface is pretty static

@khl02007
Copy link
Collaborator Author

@CBroz1 Agree that this is what we should move toward in the long run. One issue is that it's a bit hard to predict what the key of the lookup dicts should be because each NWB file can name their position object differently. More practical concern is that we must finish the manuscript within a week and this is required to demonstrate run spyglass pipeline on other labs' NWB files. I say we make your suggestion a long term goal and move forward with this quick fix for now.

@edeno
Copy link
Collaborator

edeno commented Jan 20, 2024

Have you tested this with some of our nwb files?

@khl02007
Copy link
Collaborator Author

@edeno Yes I tested one of the chimi nwb files. The output of get_all_spatial_series was the same with these changes.

@edeno
Copy link
Collaborator

edeno commented Jan 20, 2024

Please lint.

src/spyglass/utils/nwb_helper_fn.py Outdated Show resolved Hide resolved
@edeno edeno merged commit 1ae9c13 into LorenFrankLab:master Jan 20, 2024
3 checks passed
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.

3 participants