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

Fix plot_sorting_summary widget #3627

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Fix plot_sorting_summary widget #3627

merged 3 commits into from
Jan 20, 2025

Conversation

alejoe91
Copy link
Member

Fix issue introduces by #3616

@alejoe91 alejoe91 added the widgets Related to widgets module label Jan 18, 2025
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.

Where is the switch to concat? I'm just trying to understand :)

@alejoe91 alejoe91 changed the title Use pd.concat instead of df.append Fix plot_sorting_summary widget Jan 19, 2025
@alejoe91
Copy link
Member Author

Changed the title! I thought it was a different error, it turns out it was only this: https://github.com/SpikeInterface/spikeinterface/pull/3627/files#diff-66327b100012a00ce020c83d3a3ea191bbb820a0ce3ac02d0328387d14350051R319

Also fixed some tests due to the new kwargs

@alejoe91 alejoe91 changed the title Fix plot_sorting_summary widget Fix plot_sorting_summary widget Jan 19, 2025
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.

couple comments :)

backend=backend,
**self.backend_kwargs[backend],
)
# adding a missing property should raise a warning
with self.assertWarns(UserWarning):
sw.plot_sorting_summary(
self.sorting_analyzer_sparse,
unit_table_properties=["missing_property"],
displayed_unit_properties=["missing_property"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason we changed the naming? this is a breaking change? I never use this argument but do others?

Copy link
Member

Choose a reason for hiding this comment

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

I think Alessio wants to test if a properties is not present. We decided to not raise an error make user life easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

@samuelgarcia I think he means the argument!!!

Copy link
Member

Choose a reason for hiding this comment

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

I did the change in another PR.
I prefer this one which more easy to understand.
It is back compatible.
Few users I think were using it (only for sorting view).
Now it will be usable with spikeinterface-gui.

src/spikeinterface/widgets/utils.py Show resolved Hide resolved
@samuelgarcia
Copy link
Member

Cool thanks camarade.

@samuelgarcia samuelgarcia merged commit cabe66e into main Jan 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
widgets Related to widgets module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants