Add methods to sparsify and densify waveforms to `ChannelSparsity` #1985

h-mayorquin · 2023-09-13T08:01:07Z

I have been wanting to add something like this for a while and it will be useful in the context of #1982.

The idea is that if we have channel sparsity, it should be possible to use the class to compute a sparse or dense representation of a waveform or a template. Now the ChannelSparsity object is part of WaveformExtractor but it does not have a way of changing the representation at a waveform at will in the spot.

Instead, we do sparsifying operations all over the place directly with the mask:

https://github.com/h-mayorquin/spikeinterface/blob/aa5248454462084dcd7a05296ef82c0bb717a285/src/spikeinterface/core/waveform_extractor.py#L946-L950

https://github.com/h-mayorquin/spikeinterface/blob/aa5248454462084dcd7a05296ef82c0bb717a285/src/spikeinterface/core/waveform_extractor.py#L893-L909

I think it is better to centralize this operation where it semantically make sense (a sparsity class should sparsify!) and test it appropriately.

h-mayorquin · 2023-09-13T15:48:49Z

src/spikeinterface/core/sparsity.py

    def __repr__(self):
-        ratio = np.mean(self.mask)
-        txt = f"ChannelSparsity - units: {self.unit_ids.size} - channels: {self.channel_ids.size} - ratio: {ratio:0.2f}"
+        sparsity = 1 - np.mean(self.mask)


So here you rather have ratio = np.mean(self.mask) as it was before and just add the clarification that P(x=1), right?

h-mayorquin · 2023-09-13T15:49:03Z

src/spikeinterface/core/sparsity.py

@@ -119,6 +125,65 @@ def unit_id_to_channel_indices(self):
                self._unit_id_to_channel_indices[unit_id] = channel_inds
        return self._unit_id_to_channel_indices

+    def sparsify_waveforms(self, waveforms: np.ndarray, unit_index: int) -> np.ndarray:


Add some checks for dimensions.

src/spikeinterface/core/sparsity.py

alejoe91 · 2023-09-20T13:29:42Z

src/spikeinterface/core/sparsity.py

+        waveforms : np.array
+            The sparsified waveforms array of shape (num_units, num_samples, num_active_channels).


This is waveforms_or_template, right? It can be 3d (waveforms) or 2d (templates) (as in the tests). Maybe we should change the docstring and variable names accordingly. What do you think @h-mayorquin ?

I think this is a good suggesiton. I think on templates as central / canonical / representative forms of waveforms so I would like to keep the name of the variable waveforms as it is the general type. But I think that the docstring should clarify this and make it explicit.

I made some changes to the docstring. Let me know what you think.

If you think that templates should be a variable name it would still prefer to have instead a method called sparsify_template and densify_template that take care of the 2 dimensional case and separate the functions. Maybe that's even better ...

I think it's ok for now! I agree that templates are "mean" waveforms. Thanks for clarifying the docstring!

samuelgarcia · 2023-09-20T14:48:42Z

src/spikeinterface/core/sparsity.py

@@ -119,6 +125,85 @@ def unit_id_to_channel_indices(self):
                self._unit_id_to_channel_indices[unit_id] = channel_inds
        return self._unit_id_to_channel_indices

+    def sparsify_waveforms(self, waveforms: np.ndarray, unit_id: str) -> np.ndarray:


unit_id is not always a str can be int

Could be usefull also to have unit_index entry somtimes.

good point!

Yes, I forgot to do this. In fact, mea culpa, I kind of new it when I did it but I ... wished that unit_ids were only string and intentionally forgot : P

I will push a small fix for this.

I would not like to do have a method with two different type of inputs, then 1/4 of the code is used to validate the input with if cases.

samuelgarcia · 2023-09-20T14:49:07Z

src/spikeinterface/core/sparsity.py

+
+        return sparsified_waveforms
+
+    def densify_waveforms(self, waveforms: np.ndarray, unit_id: str) -> np.ndarray:


samuelgarcia · 2023-09-20T14:50:52Z

src/spikeinterface/core/sparsity.py

+        assert_msg = (
+            "Waveforms do not seem to be be in the sparsity shape of this unit_id. The number of active channels is "
+            f"{len(non_zero_indices)} but the waveform has {waveforms.shape[-1]} active channels."
+        )
+        assert self.are_waveforms_sparse(waveforms=waveforms, unit_id=unit_id), assert_msg


very little detail as a genral partice: here you format the message even before you made the assert.
But if the assert pass there is no need to make the text formatting. And so putting the message after the assert ..., is better no ?

I do feel that waste of computation but I prefer this for readability. This should not have any really impact on performance anywhere.

As an aside to your aside, it is usually considered a bad practice to use asserts the way that we do here (but most people, doing this is very popular!) because you can dissable assertions in optimize code:

https://discuss.python.org/t/mismatch-between-asserts-semantics-and-how-its-used-o-oo-disable/29282?page=2

The recommended way to do this would be to do and if statement and then raise the appropiate exception but I think that most people does not like this? What do you think?

I think this is a science vs industry thing. Every piece of science code I've ever read uses assert instead of if-exception. I think with the assumption that scientists aren't going to shut off assertions to increase speed. And using the asserts does empower the user such that if they don't want the protections designed by the developer they can shut them off to get the speed boost. But just my two cents.

h-mayorquin added 3 commits September 13, 2023 09:39

add tests for densification and sparsification in ChannelSparsity

79490e2

passing tests

75bf8e1

fix docstrings

aa52484

h-mayorquin added the core Changes to core module label Sep 13, 2023

h-mayorquin self-assigned this Sep 13, 2023

h-mayorquin added 2 commits September 13, 2023 10:13

fix docstring

99fb18d

Merge branch 'main' into sparsity_method_to_densify

f7e83e1

h-mayorquin marked this pull request as ready for review September 13, 2023 11:21

Merge branch 'main' into sparsity_method_to_densify

3f1a043

h-mayorquin commented Sep 13, 2023

View reviewed changes

h-mayorquin added 4 commits September 20, 2023 13:23

added checks

7085e77

Merge branch 'main' into sparsity_method_to_densify

de14163

better assertion message

dff698f

typo

b100cff

alejoe91 reviewed Sep 20, 2023

View reviewed changes

src/spikeinterface/core/sparsity.py Outdated Show resolved Hide resolved

alejoe91 reviewed Sep 20, 2023

View reviewed changes

src/spikeinterface/core/sparsity.py Outdated Show resolved Hide resolved

h-mayorquin added 5 commits September 20, 2023 15:09

base the implementation in unit_id instead of unit_index

5ffbb7f

Merge branch 'main' into sparsity_method_to_densify

f9640ee

better variable name

1ec93b5

alessio suggestions

54d92a2

Merge branch 'main' into sparsity_method_to_densify

a7dc63a

alejoe91 reviewed Sep 20, 2023

View reviewed changes

improve docstring

646679f

alejoe91 approved these changes Sep 20, 2023

View reviewed changes

h-mayorquin merged commit 468396a into SpikeInterface:main Sep 20, 2023

h-mayorquin deleted the sparsity_method_to_densify branch September 20, 2023 14:05

samuelgarcia reviewed Sep 20, 2023

View reviewed changes

h-mayorquin mentioned this pull request Sep 20, 2023

Correct some typing to PR 1985 #2031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to sparsify and densify waveforms to `ChannelSparsity` #1985

Add methods to sparsify and densify waveforms to `ChannelSparsity` #1985

h-mayorquin commented Sep 13, 2023

h-mayorquin Sep 13, 2023

alejoe91 Sep 13, 2023

h-mayorquin Sep 20, 2023

h-mayorquin Sep 13, 2023

h-mayorquin Sep 20, 2023

alejoe91 Sep 20, 2023

h-mayorquin Sep 20, 2023

h-mayorquin Sep 20, 2023 •

edited

Loading

alejoe91 Sep 20, 2023

samuelgarcia Sep 20, 2023

samuelgarcia Sep 20, 2023

alejoe91 Sep 20, 2023

h-mayorquin Sep 20, 2023

samuelgarcia Sep 20, 2023

samuelgarcia Sep 20, 2023

h-mayorquin Sep 20, 2023

zm711 Sep 20, 2023

		waveforms : np.array
		The sparsified waveforms array of shape (num_units, num_samples, num_active_channels).


		return sparsified_waveforms

		def densify_waveforms(self, waveforms: np.ndarray, unit_id: str) -> np.ndarray:

Add methods to sparsify and densify waveforms to ChannelSparsity #1985

Add methods to sparsify and densify waveforms to ChannelSparsity #1985

Conversation

h-mayorquin commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-mayorquin Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add methods to sparsify and densify waveforms to `ChannelSparsity` #1985

Add methods to sparsify and densify waveforms to `ChannelSparsity` #1985

h-mayorquin Sep 20, 2023 •

edited

Loading