Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-42

Refactor the image display code to remove explicit references to ds9

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None
    • Location:
      PR

      Description

      The current stack image display code assumes that you're talking to ds9:

      import lsst.afw.display.ds9 as ds9
       
      ds9.erase()

      As the IPAC team thinks about supporting firefly, and ginga gains traction in the scipy world, we need replace this interface with something more general.

      In the longer run we need to decide what APIs we need to support, but this is hard to do until we have more than one backend. This RFC proposes that we change the interface to something like:

      import lsst.afw.display.imdisplay as imdisplay
      imdisplay.setBackend('ds9')
       
      imdisplay.erase()

      It would be possible to hoist these methods to lsst.afw.display itself:

      import lsst.afw.display as afwDisplay
      afwDisplay.setBackend('ds9')
       
      afwDisplay.erase()

      A discussion of the pros and cons is in scope for the RFC.

      The choice of backend (in this case ds9) should be configurable either explicitly, as illustrated here, or via some deus ex machina technique (a dot file, an environment variable, ...). I put the backend choice explicitly in the examples, but I would not expect this to appear in utility code.

        Attachments

          Issue Links

            Activity

            No builds found.
            rhl Robert Lupton created issue -
            Hide
            rowen Russell Owen added a comment -

            I agree this should be changed, but I personally would be inclined to wait until we have another backend, so we have a better idea what a system that supports multiple backends needs to support.

            Show
            rowen Russell Owen added a comment - I agree this should be changed, but I personally would be inclined to wait until we have another backend, so we have a better idea what a system that supports multiple backends needs to support.
            Hide
            rhl Robert Lupton added a comment -

            (I'd prefer .setBackend() to .configure() now I think about it, and shall modify the RFC)

            I think we need something like this to be able to play with new backends like firefly. So I'd like:
            1. To accept this RFC (I don't think it's controversial)
            2. Prototype firefly with the current API
            3. File another RFC on a better interface based on the experience with two (or more) backends.

            There's backwards compatibility with e.g.

            import lsst.afw.display as afwDisplay
            afwDisplay.setBackend("ds9")
            ds9 = afwDisplay

            Show
            rhl Robert Lupton added a comment - (I'd prefer .setBackend() to .configure() now I think about it, and shall modify the RFC) I think we need something like this to be able to play with new backends like firefly. So I'd like: 1. To accept this RFC (I don't think it's controversial) 2. Prototype firefly with the current API 3. File another RFC on a better interface based on the experience with two (or more) backends. There's backwards compatibility with e.g. import lsst.afw.display as afwDisplay afwDisplay.setBackend("ds9") ds9 = afwDisplay
            rhl Robert Lupton made changes -
            Field Original Value New Value
            Description The current stack image display code assumes that you're talking to ds9:
            {code}
            import lsst.afw.display.ds9 as ds9

            ds9.erase()
            {code}
            As the IPAC team thinks about supporting firefly, and ginga gains traction in the scipy world, we need replace this interface with something more general.

            In the longer run we need to decide what APIs we need to support, but this is hard to do until we have more than one backend. This RFC proposes that we change the interface to something like:
            {code}
            import lsst.afw.display.imdisplay as imdisplay
            imdisplay.configure('ds9')

            imdisplay.erase()
            {code}

            It would be possible to hoist these methods to lsst.afw.display itself:
            {code}
            import lsst.afw.display as display
            display.configure('ds9')

            display.erase()
            {code}
            A discussion of the pros and cons is in scope for the RFC
            The current stack image display code assumes that you're talking to ds9:
            {code}
            import lsst.afw.display.ds9 as ds9

            ds9.erase()
            {code}
            As the IPAC team thinks about supporting firefly, and ginga gains traction in the scipy world, we need replace this interface with something more general.

            In the longer run we need to decide what APIs we need to support, but this is hard to do until we have more than one backend. This RFC proposes that we change the interface to something like:
            {code}
            import lsst.afw.display.imdisplay as imdisplay
            imdisplay.setBackend('ds9')

            imdisplay.erase()
            {code}

            It would be possible to hoist these methods to lsst.afw.display itself:
            {code}
            import lsst.afw.display as afwDisplay
            afwDisplay.setBackend('ds9')

            afwDisplay.erase()
            {code}
            A discussion of the pros and cons is in scope for the RFC
            Hide
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment -

            +1
            particularly in the context of your plan in comment 2.

            Show
            wmwood-vasey Michael Wood-Vasey [X] (Inactive) added a comment - +1 particularly in the context of your plan in comment 2.
            Hide
            tjenness Tim Jenness added a comment - - edited

            This is obviously important. The ORAC-DR display system worked exactly like this with multiple backends (actually, they were configurable by the user so the pipeline code just said "display" and the pipeline worked out what that meant). Whether it's advisable to hard code ds9 into the code at all depends on how well functionality in the different display tools is abstracted by the display engine. Again, ORAC-DR had the notion that each tool knew whether it could handle displays of images, catalogs, spectra etc.

            I know how to interface to GAIA/SkyCat if that is of interest as an additional data point.

            Show
            tjenness Tim Jenness added a comment - - edited This is obviously important. The ORAC-DR display system worked exactly like this with multiple backends (actually, they were configurable by the user so the pipeline code just said "display" and the pipeline worked out what that meant). Whether it's advisable to hard code ds9 into the code at all depends on how well functionality in the different display tools is abstracted by the display engine. Again, ORAC-DR had the notion that each tool knew whether it could handle displays of images, catalogs, spectra etc. I know how to interface to GAIA/SkyCat if that is of interest as an additional data point.
            Hide
            jbosch Jim Bosch added a comment -

            What about using a class instance instead of a module global variable to represent the connection to a particular display window? I think that's a more general interface, in that it supports connections that have more state than we'd want to stuff in global module variables, and I think it better captures the idea that we're displaying multiple images. With that approach, you'd set the backend by constructing an instance of the right class, which would all just be subclasses of a common base class.

            In the DS9 implementation of such an interface, I'm not sure if we'd want to have one instance for all frames while continuing to use a frame kwarg, or one instance for each frame; it might depend on whether other display tools we're considering have a frame or layer concept.

            Show
            jbosch Jim Bosch added a comment - What about using a class instance instead of a module global variable to represent the connection to a particular display window? I think that's a more general interface, in that it supports connections that have more state than we'd want to stuff in global module variables, and I think it better captures the idea that we're displaying multiple images. With that approach, you'd set the backend by constructing an instance of the right class, which would all just be subclasses of a common base class. In the DS9 implementation of such an interface, I'm not sure if we'd want to have one instance for all frames while continuing to use a frame kwarg, or one instance for each frame; it might depend on whether other display tools we're considering have a frame or layer concept.
            Hide
            rhl Robert Lupton added a comment -

            Re Tim Jenness's comment, are you proposing that the system guess what backend you want? I can see providing a default (maybe "guess"), but I'd have thought users would want control (maybe via a . file, of course – cf. matplotlib). For example, within a few months I hope to see ds9 and firefly as viable backends; having GAIA/skycat too would be great. I'm not sure which I'll want on a particular day.

            Show
            rhl Robert Lupton added a comment - Re Tim Jenness 's comment, are you proposing that the system guess what backend you want? I can see providing a default (maybe "guess"), but I'd have thought users would want control (maybe via a . file, of course – cf. matplotlib). For example, within a few months I hope to see ds9 and firefly as viable backends; having GAIA/skycat too would be great. I'm not sure which I'll want on a particular day.
            Hide
            swinbank John Swinbank added a comment -

            In first reading, my concern – and I think it's related to Tim Jenness's point – is that it's not obvious why:

            import lsst.afw.display as afwDisplay
            afwDisplay.setBackend('ds9')

            is any more general than

            import lsst.afw.display.ds9 as ds9

            – in both cases, you've baked into the pipeline code that you'll be using ds9 for display. That might be necessary if whatever it is you're displaying absolutely needs ds9's functionality, but it doesn't seem to be much of a win. Better is to let the user configure whichever backend they like (by means of command line argument, dotfile, whatever).

            Based on the subsequent comment, I think that was implicit in your proposal, and the hard-coded 'ds9' was just a placeholder. In which case I am happy.

            Show
            swinbank John Swinbank added a comment - In first reading, my concern – and I think it's related to Tim Jenness 's point – is that it's not obvious why: import lsst.afw.display as afwDisplay afwDisplay.setBackend('ds9') is any more general than import lsst.afw.display.ds9 as ds9 – in both cases, you've baked into the pipeline code that you'll be using ds9 for display. That might be necessary if whatever it is you're displaying absolutely needs ds9's functionality, but it doesn't seem to be much of a win. Better is to let the user configure whichever backend they like (by means of command line argument, dotfile, whatever). Based on the subsequent comment, I think that was implicit in your proposal, and the hard-coded 'ds9' was just a placeholder. In which case I am happy.
            Hide
            tjenness Tim Jenness added a comment -

            I'm saying that Frossie Economou required a system that completely separated the code requesting the display of spectrum/catalog/image/overlay from the configuration code that actually expressed the desires of the user as to what tool should be used. In fact, sometimes a display command in the pipeline code would not result in a display at all because the user had declared that a particular product was not of interest to them. All the "are you interested in product X" and "how should we display product Y" decisions were under the control of the user (instrument scientist in reality) and no code was tweaked.

            Show
            tjenness Tim Jenness added a comment - I'm saying that Frossie Economou required a system that completely separated the code requesting the display of spectrum/catalog/image/overlay from the configuration code that actually expressed the desires of the user as to what tool should be used. In fact, sometimes a display command in the pipeline code would not result in a display at all because the user had declared that a particular product was not of interest to them. All the "are you interested in product X" and "how should we display product Y" decisions were under the control of the user (instrument scientist in reality) and no code was tweaked.
            rhl Robert Lupton made changes -
            Description The current stack image display code assumes that you're talking to ds9:
            {code}
            import lsst.afw.display.ds9 as ds9

            ds9.erase()
            {code}
            As the IPAC team thinks about supporting firefly, and ginga gains traction in the scipy world, we need replace this interface with something more general.

            In the longer run we need to decide what APIs we need to support, but this is hard to do until we have more than one backend. This RFC proposes that we change the interface to something like:
            {code}
            import lsst.afw.display.imdisplay as imdisplay
            imdisplay.setBackend('ds9')

            imdisplay.erase()
            {code}

            It would be possible to hoist these methods to lsst.afw.display itself:
            {code}
            import lsst.afw.display as afwDisplay
            afwDisplay.setBackend('ds9')

            afwDisplay.erase()
            {code}
            A discussion of the pros and cons is in scope for the RFC
            The current stack image display code assumes that you're talking to ds9:
            {code}
            import lsst.afw.display.ds9 as ds9

            ds9.erase()
            {code}
            As the IPAC team thinks about supporting firefly, and ginga gains traction in the scipy world, we need replace this interface with something more general.

            In the longer run we need to decide what APIs we need to support, but this is hard to do until we have more than one backend. This RFC proposes that we change the interface to something like:
            {code}
            import lsst.afw.display.imdisplay as imdisplay
            imdisplay.setBackend('ds9')

            imdisplay.erase()
            {code}

            It would be possible to hoist these methods to lsst.afw.display itself:
            {code}
            import lsst.afw.display as afwDisplay
            afwDisplay.setBackend('ds9')

            afwDisplay.erase()
            {code}
            A discussion of the pros and cons is in scope for the RFC.

            The choice of backend (in this case ds9) should be configurable either explicitly, as illustrated here, or via some deus ex machina technique (a dot file, an environment variable, ...). I put the backend choice explicitly in the examples, but I would not expect this to appear in utility code.
            Hide
            rhl Robert Lupton added a comment -

            I clarified the RFC. Utility code should not call the "setBackend" method.

            Show
            rhl Robert Lupton added a comment - I clarified the RFC. Utility code should not call the "setBackend" method.
            Hide
            rhl Robert Lupton added a comment -

            Re Jim Bosch's comment, having separate connection objects sounds like a nice way to generalise the ds9-centric frame argument. I think you'd want to do this via a factory object, which is itself configured in the ways that we have been discussing.

            I'll include something along these lines in my prototype implementation of this RFC.

            Show
            rhl Robert Lupton added a comment - Re Jim Bosch 's comment, having separate connection objects sounds like a nice way to generalise the ds9-centric frame argument. I think you'd want to do this via a factory object, which is itself configured in the ways that we have been discussing. I'll include something along these lines in my prototype implementation of this RFC.
            Hide
            tjenness Tim Jenness added a comment -

            I'll note that for the "do I want this displayed" logic to work, there needs to be a way for the display system to understand what stage in the processing this product represents. Do the ExposureX objects, or whatever, know what's been done to them?

            Show
            tjenness Tim Jenness added a comment - I'll note that for the "do I want this displayed" logic to work, there needs to be a way for the display system to understand what stage in the processing this product represents. Do the ExposureX objects, or whatever, know what's been done to them?
            Hide
            frossie Frossie Economou added a comment -

            Yes, I am very keen on the design approach of (a) decoupling the code from a gui implementation and (b) giving control to the user as to how to tap into the display [Gregory Dubois-Felsmann got an earful of my opinions on the matter the other day]. It stands up to a wide range of scenarios and people are happy. You can have a default configuration for the display, but the code itself should be display agnostic.

            Show
            frossie Frossie Economou added a comment - Yes, I am very keen on the design approach of (a) decoupling the code from a gui implementation and (b) giving control to the user as to how to tap into the display [ Gregory Dubois-Felsmann got an earful of my opinions on the matter the other day]. It stands up to a wide range of scenarios and people are happy. You can have a default configuration for the display, but the code itself should be display agnostic.
            Hide
            rhl Robert Lupton added a comment -

            Re Tim Jenness's comment:

            I'll note that for the "do I want this displayed" logic to work, there needs to be a way for the display system to understand what stage in the processing this product represents. Do the ExposureX objects, or whatever, know what's been done to them?

            I think this is out of scope for this RFC, which is about display technology.

            The answer is no, they don't. We are not currently configuring debug displays this way (they are configured via the lsstDebug mechanism), although it sounds perfectly reasonable. It surely ties into the whole provenance system which doesn't exist yet.

            Show
            rhl Robert Lupton added a comment - Re Tim Jenness 's comment: I'll note that for the "do I want this displayed" logic to work, there needs to be a way for the display system to understand what stage in the processing this product represents. Do the ExposureX objects, or whatever, know what's been done to them? I think this is out of scope for this RFC, which is about display technology. The answer is no, they don't. We are not currently configuring debug displays this way (they are configured via the lsstDebug mechanism), although it sounds perfectly reasonable. It surely ties into the whole provenance system which doesn't exist yet.
            Hide
            rhl Robert Lupton added a comment -

            I've pushed a prototype to tickets/pr-42, with a notebook to illustrate the API in examples/imageDisplay.ipynb (I can't get nbviewer to display that notebook, I think because it's on a branch; for now see http://www.astro.princeton.edu/~rhl/imageDisplay.html).

            I adopted Jim's suggestion to make the interface use objects, and support "virtualDisplay" and "ds9"

            Show
            rhl Robert Lupton added a comment - I've pushed a prototype to tickets/pr-42, with a notebook to illustrate the API in examples/imageDisplay.ipynb (I can't get nbviewer to display that notebook, I think because it's on a branch; for now see http://www.astro.princeton.edu/~rhl/imageDisplay.html ). I adopted Jim's suggestion to make the interface use objects, and support "virtualDisplay" and "ds9"
            rhl Robert Lupton made changes -
            Resolution Done [ 10000 ]
            Status Proposed [ 10805 ] Adopted [ 10806 ]
            Hide
            xiuqin Xiuqin Wu [X] (Inactive) added a comment -

            SUI team at IPAC got together and discussed this RFC and Robert's new implementation. In general, Firefly will be able to support this with Python APIs. More details and questions later.

            Show
            xiuqin Xiuqin Wu [X] (Inactive) added a comment - SUI team at IPAC got together and discussed this RFC and Robert's new implementation. In general, Firefly will be able to support this with Python APIs. More details and questions later.
            tjenness Tim Jenness made changes -
            Link This issue is triggering DM-2709 [ DM-2709 ]
            tjenness Tim Jenness made changes -
            Status Adopted [ 10806 ] Implemented [ 11105 ]

              People

              Assignee:
              rhl Robert Lupton
              Reporter:
              rhl Robert Lupton
              Watchers:
              Frossie Economou, Gregory Dubois-Felsmann, Jim Bosch, John Swinbank, Michael Wood-Vasey [X] (Inactive), Robert Lupton, Russell Owen, Tim Jenness, Xiuqin Wu [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins Builds

                  No builds found.