search

catalystneuro / mease-lab-to-nwb

MIT License
3 stars 2 forks source link

TTL improvements #48

Closed lkeegan closed 2 years ago

lkeegan commented 2 years ago

key changes:

CodyCBakerPhD commented 2 years ago

@lkeegan Thanks for the lovely code!

While it's fine to explicitly specify the IntervalSeries timestamps field, note that you may also be able to use the add_interval function (https://github.com/catalystneuro/nwb-conversion-tools/blob/master/tests/test_gin.py) in a bit more direct fashion.

Also, if you could remind me what issue you were having with TimeIntervals? The times there should be supported as floats equivalently.

Otherwise, if the NWBFiles output by these changes look good to you, then can merge to stay up to date!

lkeegan commented 2 years ago

While it's fine to explicitly specify the IntervalSeries timestamps field, note that you may also be able to use the add_interval function (https://github.com/catalystneuro/nwb-conversion-tools/blob/master/tests/test_gin.py) in a bit more direct fashion.

Thanks! Have changed to using this instead: https://github.com/catalystneuro/mease-lab-to-nwb/pull/48/commits/819e8c6a407693f02563f761645e8c8507f20ad9

Also, if you could remind me what issue you were having with TimeIntervals? The times there should be supported as floats equivalently.

I think the issue was that the timestamps were being generated from tick counts divided by sampling frequency, and as both of these were integers it was doing integer division instead of floating point i.e. the times all got rounded down to the nearest second (before being passed to the TimeIntervals object):

https://github.com/catalystneuro/mease-lab-to-nwb/blob/5da0994e90996f0e597f4e150aff192a6994a5e1/mease_lab_to_nwb/convert_ced/cedstimulusinterface.py#L61

CodyCBakerPhD commented 2 years ago

Going to go ahead and merge this so the PR doesn't get too clogged - once the GIN stuff is worked out we can add that in later.

Thanks for the code, and for opening the PR here so we can stay up to date!