-
Notifications
You must be signed in to change notification settings - Fork 370
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
Remove pylab imports #1302
Remove pylab imports #1302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good
@@ -6,7 +6,8 @@ | |||
|
|||
import cv2 | |||
import numpy as np | |||
import pylab as pl | |||
import matplotlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to keep alphabetical order this should go above numpy
caiman/tests/comparison_humans.py
Outdated
@@ -26,7 +26,7 @@ | |||
import numpy as np | |||
import os | |||
import time | |||
import pylab as pl | |||
import matplotlib.pyplot as plt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scoot up to keep alphabetical order
I believe that's everything: I changed the I ran through demos and they seemed fine. |
Description
Will close out item on Roadmap (#1142 ).
Pylab is a relic of the days when Python was trying to be like Matlab and its use is strongly discouraged even in its source code. This gets rid of it.
It includes things like:
And it makes Caiman less readable with
pl.foo()
syntax whereas people expectplt.foo()
when using the recommended api.Type of change
I'll be testing out things as I go through the different modules.
One question: should we change the name of the back end in
movies.py
frompylab
tomatplotlib
? It seems yes. And, should we warn people?My guess is nobody uses it, I have tested it (it works): the quality is horrible, we never recommend it -- in fact we literally throw a warning when people even try to use it. My inclination is just to make this change in the name of the back end. But if others thing we should give warnings at first, and change the name in a couple of cycles, I'd listen.
(Aside: I think a more important question is whether we even want this matplotlib interface for movies in there in the first place, but that's a bigger issue outside the scope of this PR. It seems like a maintenance burden not worth having.)