UDST / pandana

Pandas Network Analysis by UrbanSim: fast accessibility metrics and shortest paths, using contraction hierarchies :world_map:
http://udst.github.io/pandana
GNU Affero General Public License v3.0
387 stars 83 forks source link

Please do not change my matplotlib backend #132

Closed DavidMStraub closed 4 years ago

DavidMStraub commented 4 years ago

Thanks for this package!

I only have one problem with it: at the top of network.py, you are setting

matplotlib.use('Agg')

According to the comment, this is to fix Travis. I guess you are switching to a backend that doesn't need an X server. However, this can be very disruptive to anybody importing your library, e.g. in a Jupyter notebook, where plots stop appearing:

UserWarning: Matplotlib is currently using agg, which is a non-GUI backend,
so cannot show the figure

It took me quite a while to find out that this line from pandana was the culprit. Please remove it.

If it's just about testing on Travis, you can just set the display, it works well, I do it too.

Thanks!

smmaurer commented 4 years ago

Hi @DavidMStraub, thanks for pointing this out! I didn't know it was in there. Definitely agree that we should remove it.

To dos:

ozak commented 4 years ago

Any updates on this? While mostly just annoying, it messes up any notebook you create for presentations or courses. Looking at the code it seems to have been added as a hack for fixing the Travis build. A hack in the meantime is to run

import warnings
warnings.filterwarnings("ignore", category=UserWarning)

to eliminate the warning. Not optimal of course.

smmaurer commented 4 years ago

Hi @DavidMStraub and @ozak, thanks for the reminder about this. I just opened a PR to fix it: #135.

Would one of you be able to help with a little testing, to make sure it fixes the issue?

You could either (1) send me a notebook that demonstrates the problem, or (2) install the new Pandana branch and check it on your own machine.

You could install the branch like this:

git clone --branch fix-matplotlib-backend https://github.com/udst/pandana
cd pandana
conda env create -n pandana-test python=3.8 cython numpy clang llvm-openmp jupyter
conda activate pandana-test
pip install -e .

Thanks again for reporting the issue. If things go smoothly the fix should be up on pip/conda next week, along with some other updates.

ozak commented 4 years ago

I was getting the issue via pysal. It seems the issue is resolved in the latest version of pysal-2.2.0.

knaaptime commented 4 years ago

that's because we changed to a lazy import structure, so if you aren't using a handful of specific functions from segregation, then pandana never gets imported. Should be testable following a similar strategy as @jreades earlier in the thread

smmaurer commented 4 years ago

Thanks @knaaptime! I just tried this and was able to reproduce the behavior and also confirm that the new PR fixes it. Sorry for all the problems this caused.

knaaptime commented 4 years ago

no worries, this is great. thanks for the fix :)

smmaurer commented 4 years ago

Resolved in PR #135.