esi-neuroscience / oephys2nwb

Export Open Ephys binary data to NWB 2.0
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

Crashes when channels have been remapped #8

Closed KatharineShapcott closed 1 year ago

KatharineShapcott commented 2 years ago

Seems that this error is overkill, maybe we need to rethink this? With a channel remapping it is expected behaviour that they don't match.

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Input In [1], in <cell line: 3>()
      1 from oephys2nwb import export2nwb
----> 3 export2nwb('/as/projects/OWzeronoise/Sparse_Noise/345/20220720/Flash/002/', '/mnt/hpc_slurm/projects/OWzeronoise/Sparse_Noise/345/20220720/Flash/002/Sparse_Noise-345-20220720-Flash-002.nwb')

File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/pydantic/decorator.py:40, in pydantic.decorator.validate_arguments.validate.wrapper_function()

File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/pydantic/decorator.py:134, in pydantic.decorator.ValidatedFunction.call()

File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/pydantic/decorator.py:201, in pydantic.decorator.ValidatedFunction.execute()

File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/oephys2nwb/export2nwb.py:591, in export2nwb(data_dir, output, session_description, identifier, session_id, session_start_time, trial_start_times, trial_stop_times, trial_tags, trial_markers, experimenter, lab, institution, experiment_description)
    588     raise ValueError(err)
    590 # All remaining error checks (except for basic type matching) is performed by `EphysInfo`
--> 591 eInfo = EphysInfo(data_dir,
    592                   session_description=session_description,
    593                   identifier=identifier,
    594                   session_id=session_id,
    595                   session_start_time=session_start_time,
    596                   experimenter=experimenter,
    597                   lab=lab,
    598                   institution=institution,
    599                   experiment_description=experiment_description)
    601 # If `data_dir` contains multiple recordings, prepare base-name of NWB containers
    602 if len(eInfo.recordingDirs) > 1:

File <string>:16, in __init__(self, data_dir, session_description, identifier, session_id, session_start_time, experimenter, lab, institution, experiment_description, jsonFile)

File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/oephys2nwb/export2nwb.py:81, in EphysInfo.__post_init__(self)
     77     self.session_description = os.path.basename(self.data_dir)
     79 self.process_xml()
---> 81 self.process_json()
File /cs/departmentN5/conda_envs/oesyncopy/lib/python3.8/site-packages/oephys2nwb/export2nwb.py:287, in EphysInfo.process_json(self)
    285 if name != chan.get("name"):
    286     err = "Recording channel name mismatch in JSON file {}: expected {} found {}"
--> 287     raise ValueError(err.format(recJson, chan.get("name"), name))
    288 if sourceIdx != xmlIdx and recIdx != xmlIdx:
    289     err = "Recording channel index mismatch in JSON file {}: expected {} found {} or {}"

ValueError: Recording channel name mismatch in JSON file /as/projects/OWzeronoise/Sparse_Noise/345/20220720/Flash/002/Record Node 105/experiment1/recording1/structure.oebin: expected CH1 found CH16

Here's how a channel mapped file looks in our version of the GUI (renamed from .oebin to .txt for upload):

structure.txt

KatharineShapcott commented 2 years ago

@pantaray Can I just remove this check or will it break things down the line?

pantaray commented 2 years ago

Hi @KatharineShapcott ! Hm, that's an interesting question. As far as I see, removing the check won't break anything obvious, however, this line might be a problem: https://github.com/esi-neuroscience/oephys2nwb/blob/b4880d596fec6790c2aafe483feb9cf9d440782f/oephys2nwb/export2nwb.py#L695 The order of the channels for the NWB file is implicitly inferred from the XML channel ordering. Do you want to change this and use the JSON file as reference instead?

KatharineShapcott commented 2 years ago

Hmmm I looked into it some more, it seems the information is in the XML file. But you would need to use the information about whether the RecordNode came before or after the ChannelMapping Editor to know which values to take. It seems the JSON file contains the information about where the channels in that recording come from.

I'll do a test later today with a single recording with two record nodes, one before and one after the channel mapping. That should allow us to write something future proof. I suspect the information in the JSON file will be more useful for that.

Here's what the XML information looks like:

<EVENTCHANNEL name="0" number="0"/>
      <EDITOR isCollapsed="0" displayName="Channel Map" Type="ChannelMappingEditor">
        <SETTING Type="visibleChannels" Value="72"/>
        <CHANNEL Number="0" Mapping="16" Reference="-1" Enabled="1"/>
        <CHANNEL Number="1" Mapping="20" Reference="-1" Enabled="1"/>
        <CHANNEL Number="2" Mapping="19" Reference="-1" Enabled="1"/>
        <CHANNEL Number="3" Mapping="24" Reference="-1" Enabled="1"/>
        <CHANNEL Number="4" Mapping="21" Reference="-1" Enabled="1"/>
        <CHANNEL Number="5" Mapping="26" Reference="-1" Enabled="1"/>
        <CHANNEL Number="6" Mapping="29" Reference="-1" Enabled="1"/>

settings.txt

KatharineShapcott commented 2 years ago

It looks like we can get the information more easily from the JSON file. Otherwise in the XML file we need to use the "pluginIndex" to see whether the channels were mapped or not.

2022-08-09_11-59-59.zip

My idea would be instead of making an error to set the channel name and number at this point to be the ones from the JSON file and to print that we're doing this. I'll make a pull request, see what you think.