get_spiking_v1_merge_ids invalid restriction

[CBroz1]

spyglass/src/spyglass/spikesorting/v1/utils.py

Lines 37 to 71 in 03e3996

	def get_spiking_sorting_v1_merge_ids(restriction: dict):
	"""
	Parses the SpikingSorting V1 pipeline to get a list of merge ids for a given restriction.
	Parameters
	----------
	restriction : dict
	A dictionary containing some or all of the following key-value pairs:
	nwb_file_name : str
	name of the nwb file
	interval_list_name : str
	name of the interval list
	sort_group_name : str
	name of the sort group
	artifact_param_name : str
	name of the artifact parameter
	curation_id : int, optional
	id of the curation (if not specified, uses the latest curation)
	Returns
	-------
	merge_id_list : list
	list of merge ids for the given restriction
	"""
	# list of recording ids
	recording_id_list = (SpikeSortingRecordingSelection() & restriction).fetch(
	"recording_id"
	)
	# list of artifact ids for each recording
	artifact_id_list = [
	(
	ArtifactDetectionSelection() & restriction & {"recording_id": id}
	).fetch1("artifact_id")
	for id in recording_id_list
	]

get_spiking_v1_merge_ids uses the same restriction for both SpikeSortingRecordingSelection and ArtifactDetectionSelection tables. A valid restriction for one may not be valid for the other. Should these be separated into two inputs, recording_restr and artifact_restr?

The collected recording IDs are then used with fetch1 which implies that there should be only one recording per entry in ArtifactDetectionSelection. This may be true, but is not enforced, as a secondary key.

This function could also be refactored to reduce the number of fetches. Perhaps this is replaced by long distance restriction?

[samuelbray32]

You're right, the 1-1 mapping logic between recordings and artifacts assumed here is not guaranteed and needs corrected

I think this function still offers a utility over long-distance restriction as the goal is to let the user restrict from columns in multiple tables in one call (e.g the nwb_file_name sort_interval and sorter might need selected at once if want to pull out only the clusterless sortings for an epoch)