spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

Add error message and tests for `plot_motion`.

Open JoeZiminski opened this issue 1 year ago • 2 comments

This PR closes #2725 by adding an error message when ax or axes arguments are passed, these are backend_kwargs which are not supported for this function.

This is slightly suboptimal as the documentation still says they are accepted, but at least it is clear what to do when it does go wrong. I looked into making the function support passing axes but it would make things very complicated for (I don't think?) not too much benefit.

I also added tests for the function and made a couple of minor changes:

  • Added a little more explanation to the docstrings..
  • renamed the variables x and y to make slightly more descriptive.
  • renamed x-axis label Times [s] to Time [s].

I had a couple of questions:

  • The motion_lim is documented as Tuple or None but it actually expects an int. I guess this could be changed to accept a Tuple similar to depth_lim?
  • at the moment the widget object .ax is set to an empty Axes object. Maybe it would be clearer to set this to None as it is unused?

Also a more general question, the matplotlib and scipy imports are not at the top of the file, and I see the tests do not expect scipy to be installed. I guess the logic is the same for matplotlib which is why it is not top of file? Personally I would add any 'core' python modules to the general dependencies and then there is less maintenance required to have to workaround users potentially not having them installed. I don't think anyone will have trouble installing matplotlib or scipy unlike heavier modules such as numba. However may there are other considerations (e.g. docker images?).

JoeZiminski avatar May 13 '24 19:05 JoeZiminski

I had a couple of questions:

  • The motion_lim is documented as Tuple or None but it actually expects an int. I guess this could be changed to accept a Tuple similar to depth_lim?

Yeah I think that sounds super reasonable

  • at the moment the widget object .ax is set to an empty Axes object. Maybe it would be clearer to set this to None as it is unused?

Agreed

Also a more general question, the matplotlib and scipy imports are not at the top of the file, and I see the tests do not expect scipy to be installed. I guess the logic is the same for matplotlib which is why it is not top of file? Personally I would add any 'core' python modules to the general dependencies and then there is less maintenance required to have to workaround users potentially not having them installed. I don't think anyone will have trouble installing matplotlib or scipy unlike heavier modules such as numba. However may there are other considerations (e.g. docker images?).

The idea behind core VS full is that core can be used as a super lightweight module only to use the SI data model, e.g., from a third party package. Adding main Python modules to core could result in dependencies issues for these kind of 3rd party minimal usage.

alejoe91 avatar May 14 '24 07:05 alejoe91

Thanks @alejoe91! I've made the related changes, this is now ready for review.

Out of interest what kind of third-party apps need low-overhead installations? This would be useful for me to know in general when thinking about dependencies on other packages.

JoeZiminski avatar May 15 '24 14:05 JoeZiminski

Hey @samuelgarcia would you be happy with merging the changes to the motion.py in this PR (without the tests)? Assuming the changes to motion.py will not make the refactoring harder. If so I can split the changes to motion.py and the tests and add the tests back in later after refactoring.

JoeZiminski avatar Jun 24 '24 11:06 JoeZiminski

@samuelgarcia please ignore the above comment, superseded by #3068, and we can add tests later.

JoeZiminski avatar Jun 24 '24 16:06 JoeZiminski