# ds9.mtv silently fails

XMLWordPrintable

#### Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• 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.

#### Activity

Hide
Tim Jenness added a comment -

It seems that we either fix the tests to not demand ds9 or we make display_ds9 a dependency.

Show
Tim Jenness added a comment - It seems that we either fix the tests to not demand ds9 or we make display_ds9 a dependency.
Hide
Robert Lupton added a comment -

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.

Show
Robert Lupton added a comment - 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.
Hide
Robert Lupton added a comment -
 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.

Show
Robert Lupton added a comment - 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.
Hide
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.

Show
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.
Hide
Robert Lupton added a comment -

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.

Show
Robert Lupton added a comment - 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.

#### People

Assignee:
Robert Lupton
Reporter:
Paul Price
Reviewers:
Paul Price
Watchers:
Paul Price, Robert Lupton, Russell Owen, Tim Jenness