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.

Sign up for GitHub

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

Jump to bottom

plot_motion does not accept axes or ax argument #2725

Open
JoeZiminski opened this issue Apr 17, 2024 · 6 comments · May be fixed by #2841
Open

plot_motion does not accept axes or ax argument #2725

JoeZiminski opened this issue Apr 17, 2024 · 6 comments · May be fixed by #2841
Labels
widgets Related to widgets module

Comments

@JoeZiminski
Copy link
Contributor

The widgets.motion.plot_motion() function asserts that ax and axes is None here but the documentation says these should be accepted inputs.

Is there a reason for this? Happy to open a PR to make the function support ax / axes arguments if not.

Also the latest docs have a number of references to si.load_motion_info and si.plot_motion but it appears these have since moved, also happy to open PR ASAP but leaving here as don't want to forget.
@zm711 zm711 added the widgets Related to widgets module label Apr 17, 2024
@zm711
Copy link
Collaborator

zm711 commented Apr 17, 2024

Not a clue. @alejoe91 or @samuelgarcia would have to weigh in on this. Based on the code it looks like it generates its own sub-ax so maybe they didn't want the user to supply an ax since it needs to attach ax0, ax1 and ax2 to the figure.

@JoeZiminski
Copy link
Contributor Author

Yes indeed this makes sense, it works well with figure argument. I think in this case just a message needs to be added to assert and docs updated. Will open a PR on this if all agree.

@zm711
Copy link
Collaborator

zm711 commented Apr 17, 2024

As far as assert messaging I don't think anyone would complain about that :) But we can definitely wait for the others to weigh in.

@samuelgarcia
Copy link
Member

Hi
In si, some matplotlib plotting method accept ax or axes externally, which is convinient to construct summary figures for instance. When multiple axes it is more tedious to handle it so we put I put this assert at some places which was more easy. Unfortunatly the doc is generic about this backend_kwargs.

@zm711
Copy link
Collaborator

zm711 commented Apr 29, 2024

So I think the fix would be like what @JoeZiminski said. Add better assert messaging so the user knows this is one function that can't accept external ax/axes. Good for you @samuelgarcia ?

@samuelgarcia
Copy link
Member

yes yes of course (or making the code accept axes)

@JoeZiminski JoeZiminski linked a pull request May 13, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
widgets Related to widgets module
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add error message and tests for plot_motion. JoeZiminski/spikeinterface
3 participants
@samuelgarcia @JoeZiminski @zm711