search

catalystneuro / mease-lab-to-nwb

MIT License
3 stars 2 forks source link

Use tsync timestamps in NWBFile #20

Closed CodyCBakerPhD closed 3 years ago

CodyCBakerPhD commented 3 years ago

@bendichter Alessio added the tsync timestamp capabilities into the extractors a while back, just now getting around to allowing the converter to actually use them via conversion options.

So far, the Recording itself, the accelerometer data, and the Image series interfaces all support handling this.

One interesting case with the image series is the camera frames actually go slightly (5 frames) beyond the final timestamp of the recording, so these are filled in with the unsychronized timestamps.

Pointing this to add_multi_accelerometer for readability.

CodyCBakerPhD commented 3 years ago

@alejoe91 Since our last discussion on this topic, I've actually decided the easiest way (i.e., least amount of code) to pass those timestamps is your first suggestion of the local override of run_conversion in this converter. It still operates on the conversion_options routing framework, however, which makes it a bit easier than the other things we were considering.

CodyCBakerPhD commented 3 years ago

@bendichter Ready for review; all timestamps are now being passed and used by downstream interfaces

Also handles electrode metadata, and polished the main conversion script a bit for subject and session info.

CodyCBakerPhD commented 3 years ago

@bendichter Updates per our last couple meetings

  1. Adjusted main conversion script to work with new pipeline design.

  2. Extended only the main recording and accelerometer data to include tsync timesteps.

  3. Added static quick_write function for use in the quick save option of new pipeline; essentially a wrapper to the one-liner of NwbSortingExtractor.write_sorting, but since that is intended to potentially be the first time that NWBFile is written, it needs to be made with those minimal number of required kwargs such as description/id, so the quick_write function parses that the same automatic way it would during get_metadata, but independent of that sort of converter class use. So the call in the pipeline itself is still just one line

  4. Also removed dateparser as requirement; using strptime instead.

bendichter commented 3 years ago

Thanks @CodyCBakerPhD. Is there an advantage to making quick_write a static method over just a regular function?

CodyCBakerPhD commented 3 years ago

@bendichter Hmm... not really; it would just come down to how to import it into the environment. If not static, then it would be an import like from .syntalosnwbconverter import quick_write or something like that... would that be preferable?

bendichter commented 3 years ago

@CodyCBakerPhD I think that would be better. Static classes are useful if 1) We want to enforce a common syntax across classes (as is the case for RecordingExtractor.write_recording or 2) if the method is only expected to be used within that class. If neither of those are true, it's probably better to make it a stand-alone method.

CodyCBakerPhD commented 3 years ago

@bendichter OK, made it a general method that just gets imported directly