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

Add Template class #1982

Merged
merged 34 commits into from
Nov 15, 2023
Merged

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Sep 12, 2023

First draft. I am putting this out early to get feedback.

Right now the class implements two functionalities.

  1. Looking around in the repo, it seems that all the uses of templates assume that the object is a numpy array. The new class has functionality so this assumption is stil valid. That is, the new template class behaves like a numpy array: it can be sliced like a numpy array, and you can use any ufunct (.e.g. np.mean). For this see the test.
  2. Serialization: both json and pickle are implemented and tested.

@h-mayorquin h-mayorquin marked this pull request as ready for review September 20, 2023 21:21
@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Sep 25, 2023

@alejoe91 @samuelgarcia
This is ready for another round of review. Let me know if this is lacking any properties that you think should go here.

The only thorny issue for me is mismatch between passing a templates_array that is dense but a sparsity mask. We have some options:

  • Either throw an error.
  • Sparsify in the post-init if not sparse.
  • Let ir run and handle problems downstream.

I prefer the first but knowing that you like permisive methods and flexibility we have 3.

@alejoe91
Copy link
Member

alejoe91 commented Sep 25, 2023

@alejoe91 @samuelgarcia This is ready for another round of review. Let me know if this is lacking any properties that you think should go here.

The only thorny issue for me is mismatch between passing a templates_array that is dense but a sparsity mask. We have some options:

  • Either throw an error.
  • Sparsify in the post-init if not sparse.
  • Let ir run and handle problems downstream.

I prefer the first but knowing that you like permisive methods and flexibility we have 3.

@h-mayorquin I agree that in that case we should throw an error in the post_init

@alejoe91
Copy link
Member

alejoe91 commented Sep 28, 2023

Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

One question for the docstring. Is there a design reason why you wouldn't want nbefore and nafter next to each other in the docstring? I'm not super used to data classes in general so maybe the docstring is enforced in this way, but at least from an ease of reading for me it would make sense to have the related concepts together.

src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
from .sparsity import ChannelSparsity


@dataclass(kw_only=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One other comment based on my googling kw_only is a 3.10+ ( I think 3.8 and 3.9 are suppose to be supported).
https://docs.python.org/3/library/dataclasses.html

Copy link
Member

Choose a reason for hiding this comment

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

@h-mayorquin can you remove it then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.
This is another shortcoming of only having tests for python 3.10 only.

Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

Very minor docstring changes, then it's good to go for me! Thanks @h-mayorquin

src/spikeinterface/core/sparsity.py Outdated Show resolved Hide resolved
src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
Comment on lines +208 to +212

# If any channel is non-zero outside of the active channels, then the waveforms are not sparse
excess_zeros = waveforms[..., num_active_channels:].sum()

return int(excess_zeros) == 0
Copy link
Member

Choose a reason for hiding this comment

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

Not sure to be happy with this.
This is computationaly costy. For me the shape is enough.

Copy link
Member

Choose a reason for hiding this comment

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

ok for now. If this becomes problematic later we'll fix it.

NOTE: this assumes that the extra channels are zero-padded!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a boolean to deactivate this.

@h-mayorquin
Copy link
Collaborator Author

Thanks for the comments about the docstrings @zm711 . I drop this for a bit but I am back to it and will change them.

@h-mayorquin
Copy link
Collaborator Author

@zm711
Docstring related.
I don't have a good solution for this. n_before is required for the instantiation of the class but n_after is not. I separated parameters from the construction from the attributes in the latest docstring. If you have any suggestion or alternative models for how to do docstirng of dataclassess I am all ears.

Copy link
Collaborator

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

I think dividing the parameters like you have looks nice and clears up the confusion of having related parameters so spread about (for me at least). Thanks @h-mayorquin

src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
src/spikeinterface/core/template.py Outdated Show resolved Hide resolved
@h-mayorquin
Copy link
Collaborator Author

This is should be ready btw @alejoe91 @samuelgarcia

@alejoe91 alejoe91 merged commit bbbcdcc into SpikeInterface:main Nov 15, 2023
9 checks passed
@h-mayorquin h-mayorquin deleted the add_template_data_class branch November 15, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hybrid Related to Hybrid testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants