# Remove all explicit imports of ds9

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
12
• Sprint:
DRP S19-3, DRP S19-4
• Team:
Data Release Production

#### Description

There is code in meas_algorithms that explicitly imports ds9 as

 import lsst.afw.display.ds9 as ds9 

This has the side effect of setting ds9 to be the default backend for afwDisplay. As we now have multiple backends in the stack (ds9, firefly, ginga?, matplotlib) this is a practical as well as a theoretical problem and, moreover, switching to the afwDisplay interface is safe and easy.

#### Activity

Hide
John Swinbank added a comment -

Seems like the bulk of this work would be addressed by DM-9517 and DM-10604; looks like they fell between the cracks as Perry Gee was leaving. Perry, if you're watching: both of those tickets are marked as “reviewed” — do you have any plans to merge them, or should I ask somebody else to take over?

Even those two don't remove all (for values of “all” which I suggest we confine to lsst_distrib) explicit imports of ds9: a quick search shows there are more to be found in ip_isr, pipe_tasks, afw, obs_subaru, and undoubtedly elsewhere.

Show
John Swinbank added a comment - Seems like the bulk of this work would be addressed by DM-9517 and DM-10604 ; looks like they fell between the cracks as Perry Gee was leaving. Perry, if you're watching: both of those tickets are marked as “reviewed” — do you have any plans to merge them, or should I ask somebody else to take over? Even those two don't remove all (for values of “all” which I suggest we confine to lsst_distrib) explicit imports of ds9: a quick search shows there are more to be found in ip_isr , pipe_tasks , afw , obs_subaru , and undoubtedly elsewhere.
Hide
Lauren MacArthur added a comment -

This ticket is being pair programmed by team Yusra AlSayyad and Lauren MacArthur.

Show
Lauren MacArthur added a comment - This ticket is being pair programmed by team  Yusra AlSayyad and Lauren MacArthur .
Hide
Robert Lupton added a comment -

Please don't use the getDisplay interface to do this (I've no idea if you were planning to).  I added it as a way to convert over old code, but now afwDisplay has replaced that.  So there's no need to support the frame=XXX and display=YYY dance; just dump the frame support.

Show
Robert Lupton added a comment - Please don't use the getDisplay interface to do this (I've no idea if you were planning to).  I added it as a way to convert over old code, but now afwDisplay has replaced that.  So there's no need to support the frame=XXX and display=YYY dance; just dump the frame support.
Hide
Yusra AlSayyad added a comment - - edited

Just so I understand you right, you're recommending instantiating displays with afwDisplay.Display() instead of afwDisplay.getDisplay?

Show
Yusra AlSayyad added a comment - - edited Just so I understand you right, you're recommending instantiating displays with afwDisplay.Display() instead of afwDisplay.getDisplay ?
Hide
Lauren MacArthur added a comment - - edited

Ok, I think this is finally ready to go.  I don't think a formal line-by-line review is at all warranted for this, but if Robert Lupton could have a look at a few of the commits titled "Remove explicit imports of ds9" to make sure we have complied with his vision for the use of the current display interface, that would be ideal.  (You are, of course, most welcome to look an any/all of the other commits!)

The concise summary of the changes made is that, for a standalone mtv call:

 import lsst.afw.display.ds9 as ds9   ds9.mtv(image, frame=0) 

turned into

 import lsst.afw.display as afwDisplay   afwDisplay.Display(frame=0).mtv(image, title="Image") 

but if subsequent over-plotting (with dot, for example) was added, then it would look like:

 disp = afwDisplay.Display(frame=frame) disp.mtv(image, title="Image")   with disp.Buffering():  for foot in fs.getFootprints():  for p in foot.getPeaks():  disp.dot("+", p.getIx(), p.getIy(), size=0.4, ctype=afwDisplay.RED) 

All instances of ds9 were identified by doing a search on ds9 in lsst github (I'm pretty sure I've got them all, but only being able to search on the master branch makes this difficult to confirm...).  The testing of these updates was performed along the way doing the following:

for tests: set display = True and run the test, --fix bitrot-- update display code, run test, reset to display = False (usually by just deleting the added display = True)

for examples: run example --fix bitrot-- update display code, run example

for algorithms:  I ran processCcd.py with --debug using this debug.py file (I'm pretty sure it exercises all possible instances, but do have a look to see if you spot any omissions). I ran targeting both the psfex and pca psf modelers.

There were a few examples that could not be formally tested due to bit rot beyond any hope of fixing within the scope of this ticket (e.g. bin.src/psfex.py in meas_extensions_psfex – a PM was sent to Robert Lupton about this one on Slack). There were also a few cases that I don't have the expertise to test, e.g. showFootprints.py in synpipe, but they were all trivial fixes, so I have reasonable confidence they are all fine.

There were also a few "funky" behaviors I'll point out in case they are red herrings (but, unless an entirely trivial update is identified, I will leave it up to you whether further investigation on another ticket is warranted):

1.  In tests/test_mask.py in afw, when I first converted to the current display conventions, nothing would end up getting displayed in my display interface (ds9) despite having set display = True.  However, if I also included a direct import of

 import lsst.afw.display.ds9 as ds9 # noqa for some reason images don't display without both imports 

which is NEVER actually used (hence the noqa), the images do get displayed.

2. When running bin.src/displayCamera.py with --showRaft in obs_subaru, I get a

 Segmentation fault

. This is remedied by increasing the binSize from 1 to 4 in the call to cameraGeomUtils.showCamera(), for which there is no loss since the pixel values are all set to the same value (some multiple of the gain) – this also results in a much faster run-time, so I made the same change in the equivalent script in obs_cfht (even though there was no segfault there).

Show
Hide
Lauren MacArthur added a comment - - edited
Show
Lauren MacArthur added a comment - - edited Full list of PRs : https://github.com/lsst/afw/pull/437 https://github.com/lsst/analysis/pull/3 https://github.com/lsst/doc/pull/1 https://github.com/lsst/meas_algorithms/pull/152 https://github.com/lsst/meas_astrom/pull/113 https://github.com/lsst/meas_base/pull/138 https://github.com/lsst/meas_extensions_astrometryNet/pull/19 https://github.com/lsst/meas_extensions_photometryKron/pull/34 https://github.com/lsst/meas_extensions_psfex/pull/44 https://github.com/lsst/obs_cfht/pull/57 https://github.com/lsst/obs_subaru/pull/181 https://github.com/lsst/pipe_tasks/pull/274 https://github.com/lsst/synpipe/pull/18 https://github.com/lsst/ip_diffim/pull/112 https://github.com/lsst/ip_isr/pull/76 Jenkins is happy .
Hide
Robert Lupton added a comment -

Looks good.

The weirdness with tests/test_mask.py is due to a failed import, ostensibly due to ImportError: generic_type: type "LanczosWarpingKernel" is already registered!, but let's not hold this up to resolve that.

Show
Robert Lupton added a comment - Looks good. The weirdness with tests/test_mask.py is due to a failed import, ostensibly due to ImportError: generic_type: type "LanczosWarpingKernel" is already registered! , but let's not hold this up to resolve that.
Hide
Lauren MacArthur added a comment -

Thanks Robert! I did one final rebase and Jenkins. All 15 branches merged to master.

Show
Lauren MacArthur added a comment - Thanks Robert! I did one final rebase and Jenkins . All 15 branches merged to master.

#### People

Assignee:
Lauren MacArthur
Reporter:
Robert Lupton
Reviewers:
Robert Lupton
Watchers:
Gabor Kovacs [X] (Inactive), John Swinbank, Lauren MacArthur, Robert Lupton, Yusra AlSayyad