Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No curation ephys AND kilosort triggering #36

Closed
wants to merge 89 commits into from

Conversation

ttngu207
Copy link
Contributor

@ttngu207 ttngu207 commented Sep 30, 2021

Fix #34
Fix #40
Fix #37

# search session dir and determine acquisition software
for ephys_pattern, ephys_acq_type in zip(['*.ap.meta', '*.oebin'],
['SpikeGLX', 'Open Ephys']):
ephys_meta_filepaths = [fp for fp in sess_dir.rglob(ephys_pattern)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ephys_meta_filepaths = [fp for fp in sess_dir.rglob(ephys_pattern)]
ephys_meta_filepaths = list(sess_dir.rglob(ephys_pattern))

for ephys_pattern, ephys_acq_type in zip(['*.ap.meta', '*.oebin'],
['SpikeGLX', 'Open Ephys']):
ephys_meta_filepaths = [fp for fp in sess_dir.rglob(ephys_pattern)]
if len(ephys_meta_filepaths):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(ephys_meta_filepaths):
if ephys_meta_filepaths:


# add additional columns to the electrodes table
electrodes_query = probe.ProbeType.Electrode * probe.ElectrodeConfig.Electrode
for additional_attribute in ['shank', 'shank_col', 'shank_row']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shank would probably map to electrode_group

# add additional columns to the units table
units_query = ephys.EphysRecording * ephys.ClusteringTask @ ephys.CuratedClustering.Unit
for additional_attribute in ['cluster_quality_label', 'spike_count', 'sampling_rate',
'spike_times', 'spike_sites', 'spike_depths']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spike_sites would probably map to electrodes

if ephys_device_name in nwbfile.devices
else nwbfile.create_device(name=ephys_device_name))

electrode_group = nwbfile.create_electrode_group(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would create one electrode group per shank


# ---- Local Field Potential ----
if ephys.LFP.Electrode & curated_clustering_key:
nwb_lfp = pynwb.ecephys.LFP(name=f'probe_{electrode_config["probe"]} - LFP')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if you need to name this according to a probe. The underlying ElectricalSeries will have an electrodes attribute that points to a subregion of the electrodes table


nwb_lfp.create_electrical_series(
name='processed_electrical_series',
data=lfp_data * 1e-6, # convert to Volts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't convert to volts, especially if the data is ints. Instead, include 1e-6 as a conversion factor.

Comment on lines +60 to +76
neuropixels_probes_config = {
'neuropixels 1.0 - 3A': dict(site_count=960, col_spacing=32, row_spacing=20,
white_spacing=16, col_count=2,
shank_count=1, shank_spacing=0),
'neuropixels 1.0 - 3B': dict(site_count=960, col_spacing=32, row_spacing=20,
white_spacing=16, col_count=2,
shank_count=1, shank_spacing=0),
'neuropixels UHD': dict(site_count=384, col_spacing=6, row_spacing=6,
white_spacing=0, col_count=8,
shank_count=1, shank_spacing=0),
'neuropixels 2.0 - SS': dict(site_count=1280, col_spacing=32, row_spacing=15,
white_spacing=0, col_count=2,
shank_count=1, shank_spacing=250),
'neuropixels 2.0 - MS': dict(site_count=1280, col_spacing=32, row_spacing=15,
white_spacing=0, col_count=2,
shank_count=4, shank_spacing=250)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to use ProbeInterface for this

electrodes_query = probe.ProbeType.Electrode * probe.ElectrodeConfig.Electrode
for additional_attribute in ['shank', 'shank_col', 'shank_row']:
nwbfile.add_electrode_column(
name=electrodes_query.heading.attributes[additional_attribute].name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A column named "name" might be illegal. Could we change this to "label"?

for additional_attribute in ['cluster_quality_label', 'spike_count', 'sampling_rate',
'spike_times', 'spike_sites', 'spike_depths']:
nwbfile.add_unit_column(
name=units_query.heading.attributes[additional_attribute].name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here again, "name" might be an illegal name for a column

# add additional columns to the units table
units_query = ephys.EphysRecording * ephys.ClusteringTask @ ephys.CuratedClustering.Unit
for additional_attribute in ['cluster_quality_label', 'spike_count', 'sampling_rate',
'spike_times', 'spike_sites', 'spike_depths']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the following error regarding spike times:

UserWarning: Column 'spike_times' is predefined in Units with index=True which does not match the entered index argument. The predefined index spec will be ignored. Please ensure the new column complies with the spec. This will raise an error in a future version of HDMF.
warn(msg)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to add 'spike_times', it is by default part of the Units table. Just omit "spike_times" this from the add_unit_column calls and keep it in the add_unit calls

@ttngu207
Copy link
Contributor Author

I suggest closing this PR, removing NWB export and issue a new PR.
As NWB export is implemented and PR'ed on #44 .

A fresh PR would also make it easier to review.
Thoughts @kabilar @CBroz1 ?

@kabilar
Copy link
Collaborator

kabilar commented Jan 21, 2022

Hi @ttngu207,

Happy to review in this pull request or a new one. You're right. We should remove the NWB export from the pull request. I will review this work next week.

@ttngu207
Copy link
Contributor Author

Per discussion with @kabilar, I'm closing this PR, removing the NWB export, and reissue as new PR

@ttngu207 ttngu207 closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants