spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

Add name as an extractor attribute for `__repr__` purposes

Open h-mayorquin opened this issue 1 year ago • 15 comments

I am working on the tutorial for the upcoming school and I think that the following could be useful quality of live improvement for talking about notebooks.

Right now the name of an extractor object displayed by the repr in the notebooks is the name of the class:

image

This means that two different objects will have the same name and I can't differentiate them by their repr. A limitation when presenting notebooks and I want to talk about two different recordings of the same class.

Ti addresss this, this PR introduces another attribute name (but I think displayed name or id could be better?) that displayed by the repr when set (otherwise it falls back to the current default of using the class name).

I also added other names to common generated objects that I think make more sense (as they don't leak implementation details).

image

h-mayorquin avatar Apr 29 '24 23:04 h-mayorquin

Hi. I am Ok with but I would put name as instance attribute not class attribute. In the generate example you give your modifying externally ythe class attribute. No ?

samuelgarcia avatar Apr 30 '24 07:04 samuelgarcia

Hi. I am Ok with but I would put name as instance attribute not class attribute. In the generate example you give your modifying externally ythe class attribute. No ?

@samuelgarcia Yes, you are correct that this are the intended semantics of the python object model. Class attributes are not mean to be modified. Thanks for the catch!

h-mayorquin avatar Apr 30 '24 13:04 h-mayorquin

Yeah, now that I think about it, I did it because some classess have that class attribute. SpikeGLX:

https://github.com/SpikeInterface/spikeinterface/blob/b6c2c91ecfc71dfa44213eaa30bf8dacf2da72a7/src/spikeinterface/extractors/neoextractors/spikeglx.py#L49-L50

h-mayorquin avatar Apr 30 '24 14:04 h-mayorquin

What about using alias instead of name? so we don't change the class attribute but we can still assign custom names to the objects. Also, what is the behavior when processing?

When filtering etc, should the alias/name be propagated?

alejoe91 avatar May 02 '24 09:05 alejoe91

For me as an end-user what I wanted to do is hack this so that I can "name"/"alias" my dataset such that it would be propagated (but I can understand why that would not want to be done). For example:

recording = read_intan(xx)
recording.set_name('AnimalASession1')
rec_f = bandpass_filter(recording)
rec_f
>>> 'AnimalASession1'

because for me I name my variable rec_f to mean filtered, but I can see where other users would just do rec = bandpass_filter(rec) and so it would be useful for them to quickly see what step their rec is at. I think beginner friendly would be not to propagate and to re-name each time to provide some degree of provenance, but for someone like me I would like more like a session ID so that if I have too many variables open I can quickly see which one is which.

zm711 avatar May 02 '24 10:05 zm711

OK, from the discussion today.

Yes, to propagation. Let's handle it as a property. (do you guys mean this just another attribute or do you want to make it an special attribute with @property semantics ?)

It seems that we would like to use name but some classess have a name attribute that might be used somewhere else. @DradeAW I vaguely remember that you were using an import by name functionality, is this correct? this is the only use case that we could think of.

h-mayorquin avatar May 02 '24 16:05 h-mayorquin

Here one place where we are using name:

https://github.com/SpikeInterface/spikeinterface/blob/42e1f024aaa0c2af823f715a78d1d95a33807732/src/spikeinterface/preprocessing/preprocessinglist.py#L84-L85

h-mayorquin avatar May 06 '24 21:05 h-mayorquin

Let's move forward with this by:

  • use the name and dump it in the annotations
  • having setters/getters to interact with the annotation

This will make sure that it's also propagated to child objects.

alejoe91 avatar May 13 '24 10:05 alejoe91

Ok @DradeAW will confirm tomorrow, but he told me he's not using .name in Lussac.

So my proposal is to get rid of the name class atributes (also in all extractors and preprocessors) and to use @property name with setters and getters to make a name annotation (so it's automatically propagated to child objects and stored).

Do we all agree? @h-mayorquin @samuelgarcia @zm711

We will also get rid of the recording_extractor_full_dict and company

alejoe91 avatar May 14 '24 12:05 alejoe91

+1 on the proposal.

h-mayorquin avatar May 14 '24 13:05 h-mayorquin

Sounds good to me!

zm711 avatar May 14 '24 13:05 zm711

I can take care of this but I will wait for @samuelgarcia to move forward.

h-mayorquin avatar May 14 '24 16:05 h-mayorquin

@samuelgarcia is ok with this! We discussed yesterday :)

alejoe91 avatar May 14 '24 17:05 alejoe91

All right, I will move forward with this!

h-mayorquin avatar May 14 '24 17:05 h-mayorquin

Ok @DradeAW will confirm tomorrow, but he told me he's not using .name in Lussac

I can confirm that this PR is fine with me :)

DradeAW avatar May 15 '24 08:05 DradeAW

@h-mayorquin @samuelgarcia after https://github.com/SpikeInterface/spikeinterface/pull/3153 and as discussed, I made a property name which is stored in the annotations.

Making it a _main_annotation propagates them to dictionary and children by default. Also added a small repr chang for sampling frequency (as discussed): below 10kHz, it gets now printed as Hz.

This is good to go for me!

alejoe91 avatar Jul 08 '24 10:07 alejoe91