Add name as an extractor attribute for `__repr__` purposes
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:
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).
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 ?
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!
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
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?
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.
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.
Here one place where we are using name:
https://github.com/SpikeInterface/spikeinterface/blob/42e1f024aaa0c2af823f715a78d1d95a33807732/src/spikeinterface/preprocessing/preprocessinglist.py#L84-L85
Let's move forward with this by:
- use the
nameand 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.
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
+1 on the proposal.
Sounds good to me!
I can take care of this but I will wait for @samuelgarcia to move forward.
@samuelgarcia is ok with this! We discussed yesterday :)
All right, I will move forward with this!
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 :)
@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!