Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_algorithms
-
Labels:
-
Story Points:1
-
Epic Link:
-
Sprint:Alert Production S17 - 12
-
Team:Alert Production
Description
meas_algorithms has several "naked" matplotlib imports that happen at the top level, and thus prevent anything that imports meas_algorithms from subsequently setting the backend. A simple fix is to move those imports into the functions where matplotlib is used, so they only occur if matplotlib is run, and preferably wrap them in try:except and log.warn on any errors.
This is a stop-gap until 5790 is properly dealt with.
Attachments
Issue Links
Activity
Short review please (just a few lines changed)? This is the same basic thing you just fixed DM-8656.
Unfortunately I found two bugs:
In `objectSizeStarSelector.py` the global symbol `pyplot` is used twice, but is no longer available. In both cases the code reads:
if display and plotMagSize and pyplot:
|
and it can be fixed by deleting and plot. flake8 showed this.
Also you warn with log.warning in both files you changed, but the method is log.warn.
in objectSizeStarSelector.py please change pyplot to plt to modernize the code, as in import matplotlib.pyplot as plt and consider running auto-pep8 on the two files you touched (the pep8 changes should be a separate commit; I think pyplot->plt would be fine as part of the first commit or as its own, as you prefer).
I suggest you exercise the modified code before committing (by forcing it to plot), if you can find a reasonably simple way to do that. Feel free to temporarily hack up the code a bit if necessary.
Good catch on those excess pyplots: I've refactored that whole thing now.
flake8 is somewhat not helpful because of the slew of warnings at the top due to "module level import not at top of file because of the install_aliases() thing and the doxygen block around line 280: that's why I didn't see the undefined pyplot. Re-running autopep8 (it was run during the py3 conversion) wouldn't gain anything really.
Please take another look, and I'll see about exercising this plotting code (it's got a frightening number of requirements to make it go, so we'll see).
Done. I see what you mean about objectSizeStarSelector.py and flake8. Is there an easy way to eliminate the need to call install_aliases()? Our standards call for our code to pass the linter (e.g. by adding {{ # noqa ...}} but that seems impractical in this case.
I had a few comments on github.
After poking with it a bit, I'd rather not touch install_aliases for now, just as a safety measure: that the file doesn't pass flake8 is sort of the least of my concerns about objectSizeStarSelector...
Due to a timing problem when pushing (I forgot to push the ticket after I rebased to master) jira thinks the PR was declined, when instead it was successfully merged.
Ah-well. Ticket is done. Thanks for the review comments.
Starting this while
DM-7444is in review, so I can maybe get them in together.