Skip to content

Add errors to ensure functions in Base of widgets#2791

Merged
samuelgarcia merged 2 commits intoSpikeInterface:mainfrom
zm711:ensure-analyzer-value-error
May 2, 2024
Merged

Add errors to ensure functions in Base of widgets#2791
samuelgarcia merged 2 commits intoSpikeInterface:mainfrom
zm711:ensure-analyzer-value-error

Conversation

@zm711
Copy link
Copy Markdown
Member

@zm711 zm711 commented May 1, 2024

This came up during talking in #2782, but I think TypeError instead of ValueError no? @alejoe91 ?

And was there a reason you didn't want an error here originally @samuelgarcia @alejoe91 ?

@zm711 zm711 added the widgets Related to widgets module label May 1, 2024
@zm711
Copy link
Copy Markdown
Member Author

zm711 commented May 1, 2024

I see what the problem was. The correlograms were the exception that needed to allow the sorting to pass through. I think this solution is a bit better since it checks for sorting and if not sorting then we do the ensure which will raise an error if someone passes something random. This should be more robust to end-users potentially passing the wrong thing (in my opinion :) )

@zm711 zm711 linked an issue May 1, 2024 that may be closed by this pull request
Comment on lines +50 to +51
if not isinstance(sorting_analyzer_or_sorting, BaseSorting):
sorting_analyzer_or_sorting = self.ensure_sorting_analyzer(sorting_analyzer_or_sorting)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here you break the fact that sorting can be an input no ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually tests are passing now.

The logic for the if statement is

if base sorting => continue as base sorting
if not base sorting => ensure sorting analyzer (for sorting analyzer or mock waveform)

So both cases are addressed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok
then it is ok.

@samuelgarcia
Copy link
Copy Markdown
Member

Theses function were more here for automatic convertion (with MockWaveform) than for a script type checking.
Sometimes a hack or the type happen after.
The idea of not having an error was to be able to both Sorting or Analyzer for instance.
I think you broke this for correlogram.
I am not sure to valide 100%. Let me have a deeper look.

@alejoe91
Copy link
Copy Markdown
Member

alejoe91 commented May 2, 2024

This looks good to me

@samuelgarcia
Copy link
Copy Markdown
Member

lets merge and see.

@samuelgarcia samuelgarcia merged commit fa710f1 into SpikeInterface:main May 2, 2024
@zm711 zm711 deleted the ensure-analyzer-value-error branch May 2, 2024 17:05
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.

sortingview attribute error

3 participants