Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-3203

ds9.mtv silently fails

    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

          Hide
          tjenness 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
          tjenness 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
          rhl 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
          rhl 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
          rhl 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
          rhl 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
          price 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
          price 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
          rhl 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
          rhl 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:
              rhl Robert Lupton
              Reporter:
              price Paul Price
              Reviewers:
              Paul Price
              Watchers:
              Paul Price, Robert Lupton, Russell Owen, Tim Jenness
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel