-
Notifications
You must be signed in to change notification settings - Fork 45
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
Miscellaneous Fixes #1220
base: master
Are you sure you want to change the base?
Miscellaneous Fixes #1220
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
lgtm - Minor comments, but nothing that would prevent merge
warnings.warn( | ||
f"Multiple ElectricalSeries objects found in: {nwb_file_abspath}\n\t" | ||
+ f"Inserting only first entry in {self.full_table_name}\n\t" | ||
+ "See issue #396 for more details." |
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.
Just making a note that this PR won't resolve #396 - it looks like more work would need to be done to either add a part table or concatenate these objects
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.
Correct, this PR just solves the case where there's non ElectricalSeries
objects in acquisition
@@ -39,9 +39,9 @@ def add_member_team(common_lab, add_admin): | |||
last_name="Basic", | |||
), | |||
dict( | |||
lab_member_name="This Loner", | |||
lab_member_name="This Loner", # codespell:ignore loner |
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.
I don't think this value is ever read, so I think I'd just change this to an accepted word. Maybe Solo
? Low prioroty
Co-authored-by: Chris Broz <[email protected]>
Description
Probe.insert_from_nwbfiile()
defineprobe_id
fromprobe_description
rather thanprobe_type
ProbeType
andProbe
ElectrodeGroup.insert_from_nwbfile()
to account for the changeRaw.insert_from_nwbfile()
to work when non-ElectricalSeries objects are in nwb.acquisitionChecklist:
CITATION.cff
alter
snippet for release notes.CHANGELOG.md
with PR number and description.