spikeinterface
spikeinterface copied to clipboard
Add error message and tests for `plot_motion`.
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
xandyto make slightly more descriptive. - renamed x-axis label
Times [s]toTime [s].
I had a couple of questions:
- The
motion_limis documented asTupleorNonebut it actually expects anint. I guess this could be changed to accept aTuplesimilar todepth_lim? - at the moment the widget object
.axis set to an emptyAxesobject. Maybe it would be clearer to set this toNoneas 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?).
I had a couple of questions:
- The
motion_limis documented asTupleorNonebut it actually expects anint. I guess this could be changed to accept aTuplesimilar todepth_lim?
Yeah I think that sounds super reasonable
- at the moment the widget object
.axis set to an emptyAxesobject. Maybe it would be clearer to set this toNoneas it is unused?
Agreed
Also a more general question, the
matplotlibandscipyimports are not at the top of the file, and I see the tests do not expectscipyto be installed. I guess the logic is the same formatplotlibwhich 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 installingmatplotliborscipyunlike heavier modules such asnumba. 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.
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.
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.
@samuelgarcia please ignore the above comment, superseded by #3068, and we can add tests later.