-
Notifications
You must be signed in to change notification settings - Fork 37
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
321/process nwb2 #334
321/process nwb2 #334
Conversation
…nit_name The PatchClampSeries units are hardcoded since 6b2a628f (Hardcode units of PatchClampSeries subclasses (#1036), 2019-08-05) [1] in pynwb. Therefore we must accept and translate these units as well. [1]: NeurodataWithoutBorders/pynwb@6b2a628
The getRawDataSourceType function was used to adapt the behaviour depending on the data source. But as we now don't do that anymore, and it currently breaks wiht MIES NWBv2 which don't have experiment_description set, let's just remove it.
Nowadays the version is written as NWB-2.2.0 so we need to expect that prefix as well.
Compare pipeline outputs from nwb2 against pipeline output from nwb1 file where both nwb1 and nwb2 were produced from the same pxp data
292176b
to
115b1fb
Compare
115b1fb
to
14801b4
Compare
If you are interested, I have hundreds of MIES generated NWB2 files at /allen/aibs/technology/waynew/ephys/output/Z/ephys/20200207_nwb2/ |
self._nwb_data = val | ||
|
||
def __init__(self): | ||
""" This class does work on __init__, so this hack is used to inject |
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.
What do you mean by "class does not work on init"?
If you provide parameters like this MiesDataSet(nwb_file=NWB_file, ontology=ontology)
with nwb file and ontology then it should work.
|
||
def __init__(self): | ||
""" This class does work on __init__, so this hack is used to inject | ||
nwb_data & other dependencies. |
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.
If I understand correctly, you propose in the future to refactor DataSet classes to inject nwb_data set object and other dependencies. This is great for testing, of course, but when I pulled out the ontology from inside the DataSet classes, scientists told me that it was not convenient for them. They did not want to think about creating an ontology object and inject it when creating a DataSet. They want to have easy way of creating DataSet objects. I guess this can be done with some helper functions or alternative constructors.
obt = ds.extract_sweep_record(sweep) | ||
|
||
misses = [] | ||
for key in exp.keys(): |
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.
dictdiffer.diff function works pretty well to compare dicts
|
||
try: | ||
value = d[key] | ||
except KeyError: |
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.
d.get(key)
is imo a much more succinct and explicit way to do this- get() returns none by default, or you can specify a default value, e.g. d.get(key, None)
. It is very slightly slower though, so if this is done a whole lot maybe just leave it.
ipfx/nwb_reader.py
Outdated
@@ -111,17 +113,17 @@ def get_unit_name(stim_attrs): | |||
def get_long_unit_name(unit): | |||
if not unit: | |||
return "Unknown" | |||
if unit.startswith('A'): | |||
elif unit.startswith('A') or unit == 'amps': |
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.
is this as specific of a check we need? startswith
seems possibly ambigious...
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 specified unit names explicitly as you suggested. thanks.
Address #321