search

catalystneuro / tye-lab-to-nwb

NWB Conversion project for the Tye lab at the Salk Institute.
MIT License
0 stars 0 forks source link

Remove `ignore_timestamps_errors` flag #43

Closed weiglszonja closed 1 year ago

weiglszonja commented 1 year ago

Using ignore_timestamps_errors currently does not guarantee that data has no gaps in it. https://github.com/NeuralEnsemble/python-neo/issues/1336 I think it is better to remove completely and if we see this error we can turn it on for investigating.

CodyCBakerPhD commented 1 year ago

Wasn't this needed for a separate session https://github.com/catalystneuro/tye-lab-to-nwb/issues/29?

weiglszonja commented 1 year ago

Yes, but now since it's turned on by default in convert_session.py, we won't know whether other sessions have this issue but they were silently ignored. I hope only #29 needed this flag and the other files are fine, but we don't know.. that is the problem. as @samuelgarcia told me, the OpenEphysRawIO doesn't check for gaps when ignore_timestamps_errors=True is turned on, so for being safe it is better to collect the affected sessions first and investigate.

CodyCBakerPhD commented 1 year ago

Ah, so perhaps it should be a session-level parameter and should be turned off by default then enabled on that one session that needed it?

I know the conversion is done now, but just for precedence and future design of this kind of thing

weiglszonja commented 1 year ago

Yeah exactly for future proofing

samuelgarcia commented 1 year ago

The main problem with the option ignore_timestamps_errors=True in OpenEphysRawIO is that they are no garanty that gaps are the same across channels. Wich is a bit dangerous because the get_analogsignal_chunk rely on the fact that channels have no gaps and are aligned.

If there is a high user demand no reading this formats when they are gaps I would need to improve the IO.