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 name as an extractor attribute for __repr__ purposes #2784

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

h-mayorquin
Copy link
Collaborator

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 h-mayorquin self-assigned this Apr 29, 2024
@h-mayorquin h-mayorquin added the core Changes to core module label Apr 30, 2024
@samuelgarcia
Copy link
Member

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 ?

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 like this. I think the difference between a KilosortSorting and an NpzFolderSorting is not a giant value added if it is the same thing (also true for recordings). I would prefer the option to name mine (something like Animal1Session1Recording) that way if I'm analyzing a couple data sets I can store that information in the analysis instance and keep oriented easier.

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

h-mayorquin commented Apr 30, 2024

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
Copy link
Collaborator Author

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

h-mayorquin and others added 2 commits April 30, 2024 08:36
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
@h-mayorquin h-mayorquin marked this pull request as ready for review April 30, 2024 16:26
@alejoe91
Copy link
Member

alejoe91 commented May 2, 2024

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?

@zm711
Copy link
Collaborator

zm711 commented May 2, 2024

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.

@h-mayorquin
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@alejoe91
Copy link
Member

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
Copy link
Member

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

@h-mayorquin
Copy link
Collaborator Author

+1 on the proposal.

@zm711
Copy link
Collaborator

zm711 commented May 14, 2024

Sounds good to me!

@h-mayorquin
Copy link
Collaborator Author

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

@alejoe91
Copy link
Member

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

@h-mayorquin
Copy link
Collaborator Author

All right, I will move forward with this!

@DradeAW
Copy link
Contributor

DradeAW commented May 15, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants