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.

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

Merged
merged 11 commits into from
Mar 31, 2024
Merged

Remove pylab imports #1302

merged 11 commits into from
Mar 31, 2024

Conversation

EricThomson
Copy link
Contributor

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:

from numpy import *
from numpy.fft import *
from numpy.random import *
from numpy.linalg import *

And it makes Caiman less readable with pl.foo() syntax whereas people expect plt.foo() when using the recommended api.

Type of change

  • Nnon-breaking change which fixes an issue

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 from pylab to matplotlib? 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.)

Copy link
Member

@pgunn pgunn left a 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
Copy link
Member

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

@@ -26,7 +26,7 @@
import numpy as np
import os
import time
import pylab as pl
import matplotlib.pyplot as plt
Copy link
Member

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

@EricThomson EricThomson marked this pull request as ready for review March 21, 2024 21:08
@EricThomson
Copy link
Contributor Author

EricThomson commented Mar 21, 2024

I believe that's everything: I changed the backend keyword from pylab to pyplot (using matplotlib seemed weird and this seems closer to the original, and will discourage its use hopefully -- I thought using matplotlib would make people want to use it: I do think it will be worth considering deprecating this option at some point in the future).

I ran through demos and they seemed fine. caimanmanager tests ran fine.

@EricThomson EricThomson merged commit f29731e into dev Mar 31, 2024
3 checks passed
@pgunn pgunn deleted the remove_pylab branch May 3, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants