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

matplotlib backend set in /algorithms/spot_finding/per_image_analysis.py #795

Closed
asmit3 opened this issue May 23, 2019 · 4 comments
Closed
Assignees

Comments

@asmit3
Copy link
Member

asmit3 commented May 23, 2019

Context

While trying to debug code in dials.stills_process, I was unable to show plots using matplotlib when I put a breakpoint in the code (atleast not interactively). The reason seems to stem from

try:
import matplotlib
# http://matplotlib.org/faq/howto_faq.html#generate-images-without-having-a-window-appear
matplotlib.use("Agg") # use a non-interactive backend
from matplotlib import pyplot
except ImportError:
pyplot = None

Here the matplotlib backend is set as part of a global import which turns off interactive plotting. Thus when a program imports something from this file, the global import lines for matplotlib turn off interactive plotting (this is what's happening with dials.stills_process). In dials.stills_process this is happening indirectly through imports made from indexer.py --> max_cell.py

Question

I was wondering if there is a reason for declaring the backend at the top in per_image_analysis.py? Can anything be done to not set the backend here and instead do so when plotting is done within the program ?

Just for demonstration, here is a barebones program that does not currently show plots. This is similar to the case in dials.stills_process

from libtbx.phil import parse

phil = parse("""
  include scope dials.algorithms.indexing.indexer.index_only_phil_scope
""", process_includes=True).extract()
from matplotlib import pyplot as plt; plt.plot(range(10), range(10)); plt.show()
@Anthchirp
Copy link
Member

I think it is reasonable to move the entire import matplotlib down to the actual plotting code blocks. Then I would also drop the try:/except:/Sorry bit as the import is required and any ImportError can be dealt with upstream. That way we wouldn't even import matplotlib when the code in per_image_analysis.py is used without plotting.
@rjgildea, @ndevenish what do you think?

@Anthchirp
Copy link
Member

Doing this would make mocking harder, but I don't see any test directly importing this file, so there is no immediate downside as far as I can see.

@graeme-winter graeme-winter self-assigned this May 25, 2019
graeme-winter added a commit that referenced this issue May 25, 2019
ndevenish added a commit to ndevenish/dials-fork that referenced this issue Jun 3, 2019
This directly sets the Agg canvas instead of relying on global state.

This fixes dials#795 by still allowing GUI plots if required by the user.
ndevenish added a commit to ndevenish/dials-fork that referenced this issue Jun 3, 2019
This directly sets the Agg canvas instead of relying on global state.

This fixes dials#795 by still allowing GUI plots if required by the user.
@ndevenish
Copy link
Member

Reopening, because it seems this is throwing lots of warnings because matplotlib versions/support and backends. Need to have another look.

Might be worth backing out change and putting the other one in for now.

@ndevenish ndevenish reopened this Jun 10, 2019
ndevenish pushed a commit that referenced this issue Jun 10, 2019
Avoids setting the backend in `per_image_analysis.py` and instead relies on the caller to have set it correctly.

Putative fix for #795.
@ndevenish
Copy link
Member

I've switched to the moving commit 5f59558 which hopefully solves without the issues of the other one.

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

No branches or pull requests

4 participants