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

Tweaks to OO display interface

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      When I wrote the initial version of display_firefly I found a few minor issues in the way I'd designed the Display class; at the same time, Lauren MacArthur found some missing functions in the backward-compatibility support for ds9.

      Please fix these; note that this implies changes to afw, display_ds9, and display_firefly.

        Attachments

          Issue Links

            Activity

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

            afw and display_ds9 are pushed to tickets/DM-2849; display_firefly is pushed to master as it's never been released (and has no tests to fail) – but that was a mistake.

            $ git diff --stat master
             python/lsst/display/ds9/ds9.py | 92 ++++++++++++++++----------------------------------------------------------------------------
             1 file changed, 16 insertions(+), 76 deletions(-)
            

            $ git diff --stat master
             python/lsst/afw/display/ds9.py           |  23 +++++++++++++----
             python/lsst/afw/display/ds9Regions.py    | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
             python/lsst/afw/display/interface.py     |  49 +++++++++++++++--------------------
             python/lsst/afw/display/virtualDevice.py |  31 +++++++++++-----------
             tests/display.py                         |  10 +++++---
             5 files changed, 184 insertions(+), 52 deletions(-)
            

            Show
            rhl Robert Lupton added a comment - afw and display_ds9 are pushed to tickets/ DM-2849 ; display_firefly is pushed to master as it's never been released (and has no tests to fail) – but that was a mistake. $ git diff --stat master python/lsst/display/ds9/ds9.py | 92 ++++++++++++++++---------------------------------------------------------------------------- 1 file changed, 16 insertions(+), 76 deletions(-) $ git diff --stat master python/lsst/afw/display/ds9.py | 23 +++++++++++++---- python/lsst/afw/display/ds9Regions.py | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ python/lsst/afw/display/interface.py | 49 +++++++++++++++-------------------- python/lsst/afw/display/virtualDevice.py | 31 +++++++++++----------- tests/display.py | 10 +++++--- 5 files changed, 184 insertions(+), 52 deletions(-)
            rhl Robert Lupton made changes -
            Field Original Value New Value
            Reviewers Lauren MacArthur [ lauren ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            lauren Lauren MacArthur added a comment -

            I've started to have a look at this. I've not yet gone through in detail (got side-tracked by a request for Jim), but I thought there was some logic to fix in the _getDisplayFromDisplayOrFrame() function in utils.py (i.e. could end up with display of None if neither display nor frame is passed into other functions such as drawBBox(), which does happen in existing code).? Is this no longer the case??

            Show
            lauren Lauren MacArthur added a comment - I've started to have a look at this. I've not yet gone through in detail (got side-tracked by a request for Jim), but I thought there was some logic to fix in the _getDisplayFromDisplayOrFrame() function in utils.py (i.e. could end up with display of None if neither display nor frame is passed into other functions such as drawBBox() , which does happen in existing code).? Is this no longer the case??
            swinbank John Swinbank made changes -
            Epic Link DM-1912 [ 15945 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-4 [ 159 ]
            Team Data Release Production [ 10301 ]
            Hide
            rhl Robert Lupton added a comment -

            You're right; sorry. I've pushed a patch

            Show
            rhl Robert Lupton added a comment - You're right; sorry. I've pushed a patch
            Hide
            lauren Lauren MacArthur added a comment -

            This all looks pretty good. I've put a few comments in the commits in afw (mostly requests for commit squashing).

            I get one test failure as I think you now need to remove the references to scaleLimits and scaleType in testDs9.py (and in your notebook imageDisplay.ipynb).

            Running with --debug on a {processCcd.py}} run was successful except for a call to ds9.ds9Cmd in pcaPsfDeterminer.py in meas_algorithms. I this a backwards compatibility issue? If so, and it can be fixed, great. If not, this also appears in analysis: deblender.py and utils.py and obs_subaru: rings.py, so would need fixing there.

            Otherwise, if you get a Buildbot to pass, I'm happy for you to merge.

            Show
            lauren Lauren MacArthur added a comment - This all looks pretty good. I've put a few comments in the commits in afw (mostly requests for commit squashing). I get one test failure as I think you now need to remove the references to scaleLimits and scaleType in testDs9.py (and in your notebook imageDisplay.ipynb ). Running with --debug on a {processCcd.py}} run was successful except for a call to ds9.ds9Cmd in pcaPsfDeterminer.py in meas_algorithms . I this a backwards compatibility issue? If so, and it can be fixed, great. If not, this also appears in analysis: deblender.py and utils.py and obs_subaru: rings.py , so would need fixing there. Otherwise, if you get a Buildbot to pass, I'm happy for you to merge.
            lauren Lauren MacArthur made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rhl Robert Lupton added a comment -

            I ran buildbot and merged to master.

            Packages affected ended up being afw, meas_algorithms, and display_ds9 (where I addressed TimJ's comments).

            Show
            rhl Robert Lupton added a comment - I ran buildbot and merged to master. Packages affected ended up being afw, meas_algorithms, and display_ds9 (where I addressed TimJ's comments).
            rhl Robert Lupton made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            swinbank John Swinbank made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]
            pgee Perry Gee made changes -
            Remote Link This issue links to "Page (Confluence)" [ 12910 ] This issue links to "Page (Confluence)" [ 12910 ]

              People

              Assignee:
              rhl Robert Lupton
              Reporter:
              rhl Robert Lupton
              Reviewers:
              Lauren MacArthur
              Watchers:
              Lauren MacArthur, Robert Lupton
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.