Details
-
Type:
Bug
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw, display_ds9
-
Labels:None
-
Team:Data Release Production
Description
While debugging, I tried the age-old technique:
import lsst.afw.display.ds9 as ds9
|
ds9.mtv(exposure)
|
This does not work, and there is no indication of failure. I have been told that I need package "display_ds9". This requirement has not been advertised, and there is nothing on the standard output that indicates this.
Attachments
Activity
I don't want to make it a dependency as I think we want to allow multiple optional backends.
In the longer run I'd like to deprecate the "import lsst.afw.display.ds9" interface, but that's going to require upstream changes (Kian-Tat Lim: do you have an opinion here?), but in the short run I'll look at postponing the error until you try to talk to ds9.
| Now no tests will pass unless display_ds9 is set up. |
Fair enough. I moved the exception to the first time you try to use ds9
| The change is a bit broader than the commit message states. |
Updated to "Make "import lsst.afw.display.ds9" throw an error if ds9 is unavailable"
| By catching and raising a new exception, you're making it more difficult to debug what happened in setDefaultBackend. Can you maybe preserve the traceback? |
There's no really good way to do this, but I agree; done.
| The commit summary message is a bit long (recommended max length is 50 chars). |
It's much less than 80 chars, and it says what it needs to say.
| The layering of exception handling (e.g., "RuntimeError: Unable to set backend to lsst.display.ds9: "No module named lsst.display.ds9" (is display_ds9 setup?)" comes from three different places) reminds me of the bad old days of handling return codes in C. Is it necessary? |
It's adding more generic information at each level, which is a good thing and nothing to do with checking return codes at all levels. We have C++ support for adding messages and rethrowing, and this is the equivalent.
The changes look fine, except that CI reveals broken packages downstream: https://ci.lsst.codes/job/stack-os-matrix/2745/label=centos-6/console . There are two failed tests in meas_algorithms.
Paul Price
added a comment - The changes look fine, except that CI reveals broken packages downstream: https://ci.lsst.codes/job/stack-os-matrix/2745/label=centos-6/console . There are two failed tests in meas_algorithms. Merged to master.
The meas_algorithms issue was due to using "with Buffering()" and I fixed the code to permit this. ip_diffim also failed, due to setting the mask transparency without checking if display was true; also fixed.
Thanks for the reminder to run the buildbot from the top down; I'd run it for afw and below, but that's not really the point.
It seems that we either fix the tests to not demand ds9 or we make display_ds9 a dependency.