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

Fix for namespaces issue #2714

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

brendanjohnharris
Copy link
Contributor

Overview:

Conflicting namespaces for ecephys files (neuropixels nwb files) mean that EcephysSessions and BehaviourEcephysSessions cannot be imported simultaneously. Foregoing manual namespace loading in the NwbApi and relying on loading fromt he nwb files themselves seems to fix this.

Addresses:

Addresses #2713

Type of Fix:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to not work as expected)
  • Documentation Change

Changes:

  • Delete lines that load a conflicting namespace in the nwb_api file and the ecephys_nwb_session_api` file
  • Set the NwbApi to load namespaces from nwb files

Validation:

Screenshots:

Unit Tests:

Script to reproduce error and fix:

Configuration details:

Checklist

  • My code follows
    Allen Institute Contribution Guidelines
  • My code is unit tested and does not decrease test coverage
  • I have performed a self review of my own code
  • My code is well-documented, and the docstrings conform to
    Numpy Standards
  • I have updated the documentation of the repository where
    appropriate
  • The header on my commit includes the issue number
  • My Pull Request has the latest AllenSDK release candidate branch
    rc/x.y.z as its merge target
  • My code passes all AllenSDK tests

Notes:

@morriscb morriscb marked this pull request as ready for review August 23, 2023 00:17
@morriscb morriscb self-requested a review August 23, 2023 00:20
@morriscb
Copy link
Contributor

Hi @brendanjohnharris. Thanks for fixing this issue and apologies for the delay in getting to the PR. The change looks good as far as I can tell, but we'll wait for the unittests to complete for before merging. If you have a sec would you mind fixing the two small linting issues in the file? I know this isn't code you touched, the linter is set to run only on changed files so it picked up previous linting error from before the repo had automatic linting. Thanks!

allensdk/brain_observatory/nwb/nwb_api.py:1:1: F401 'pathlib.Path' imported but unused
allensdk/brain_observatory/nwb/nwb_api.py:27:80: E501 line too long (82 > 79 characters)

@morriscb morriscb merged commit e0aa7e3 into AllenInstitute:rc/2.16.0 Sep 8, 2023
10 of 11 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.

2 participants