BouchardLab / nsds_lab_to_nwb

Python package to convert NSDS Lab data to NWB files.
https://nsds-lab-to-nwb.readthedocs.io/en/latest/
0 stars 4 forks source link

Add electrical series description and resampling #64

Closed jthermiz closed 3 years ago

jthermiz commented 3 years ago

Added resampling software (using process_nwb) so that user can resample down to nearest kHz if they desire. By default, it will resample down to nearest kHz if the hardware sample rate is not divisible by 1000.

I also added more description per Ryan's feedback.

One issue question I had is whether there's a better way to copy or modify the electrical series? Currently, I create a new one and copy over the electrical series attributes that I believe are important / used. But I'm not sure if I captured all of them.

I implemented these changes in neural_data_originator.py so that these operations can be used for any neural manager (TDT, HTK, etc...). @JesseLivezey suggested that the electrical series creation should happen in neural_data_originator and not within the neural managers.

jihyunbak commented 3 years ago

@jthermiz Thanks for the PR!

I haven't reviewed the resampling part yet, but here's my suggestion as to where we set the description text. I suggest we return to the previous structure in which the e-series is created at the Manager level, but

I think this also fits Ryan's suggestion in the Slack channel (pass a dictionary of attributes), because if we want to pass more attributes, the keyword arguments can be easily collected into a dictionary. How does this sound?

jihyunbak commented 3 years ago

Now about resampling … if the e-series is created at the Manager level, looks like resampling should also happen at that level, to avoid creating e-series twice. Perhaps a clean solution is to make a new class NeuralDataManager that has an empty extract() method, and a resample_es() method that does what you implemented here. Then HTKManager and TDTManager can inherit NeuralDataManager and implement their own versions of extract() as they do now.

~Finally, in NeuralDataOriginator, we can call self.neural_data_manager.resample_es() when the resampling flag is true.~ EDIT: we should probably pass the resampling flag as an kwarg to NeuralDataManager.

How does this sound? @jthermiz @JesseLivezey

jihyunbak commented 3 years ago

(based on the conversation on the Slack channel, I'm revoking most of my two comments above)

jthermiz commented 3 years ago

This PR has be superseded by #78