Adapt display_firefly to new Firefly API and make py3-compatible DM-7321 #1
Conversation
|
|
||
|
|
||
| print("RHL", event) | ||
| lsst.log.debug("RHL {}".format(event)) |
There was a problem hiding this comment.
We probably don't want RHL in the debug messages.
| lsst.log.debug('*************area select') | ||
| pParams = {'URL': 'http://web.ipac.caltech.edu/staff/roby/demo/wise-m51-band2.fits', | ||
| 'ColorTable': '9'} | ||
| plot_id = 3 |
There was a problem hiding this comment.
I was unable to test the interact stuff. Looking at this hardwired URL to Trey's M51 image I wonder if it works yet?
There was a problem hiding this comment.
I did not get interact() working for this ticket. It will be addressed in DM-8173.
| _fireflyClient.addListener(self.__handleCallbacks) | ||
| _fireflyClient = firefly_client.FireflyClient("%s:%d" % (host, port), name) | ||
| _fireflyClient.launch_browser() | ||
| _fireflyClient.add_listener(self.__handleCallbacks) |
There was a problem hiding this comment.
This looks like a white-space only change; is it necessary? There are others, but I haven't commented on all of them
There was a problem hiding this comment.
Since the new LSST Python style guide hews closely to PEP8, we changed the Firefly API to use PEP8-style names (e.g. no camelCase for function names). Other whitespace changes were introduced by autopep8 as part of the Python 3 porting procedure. It was probably a rookie mistake on my part to include the Python 3 porting together with the more substantial changes in DM-7321.
|
|
||
| self._scale('Linear', 1, 99, 'Percent') | ||
| #self._scale('linear', 1, 99, 'percent') |
There was a problem hiding this comment.
Please remove commented out code
There was a problem hiding this comment.
Thanks for catching this, will do.
| exp = afwImage.ExposureF(mskimg, wcs) | ||
| else: | ||
| raise RuntimeError("Unknown image type") | ||
|
|
There was a problem hiding this comment.
You're allocating a lot of memory just to simplify the code (at the worst case, a mask image, a variance image, and misc. small things like a Wcs). I don't think we want to do this.
There was a problem hiding this comment.
I agree this is not efficient. I can try backing this out. In the stack, putting the wcs back together with the image requires building up an Exposure. I see now why you made your own displayLib.writeFitsImage to make the FITS file for Firefly.
There was a bug (since fixed) in the Firefly server in which masks in separate files didn't quite work right. I will try using displayLib.writeFitsImage to write (image + wcs) and mask as separate FITS files and will upload those to Firefly.
| @@ -183,23 +222,25 @@ def _drawLines(self, points, ctype): | |||
| def _erase(self): | |||
| """Erase the specified DS9 frame""" | |||
There was a problem hiding this comment.
Thanks for catching this, will do.
| elif min == "zscale": | ||
| pass | ||
| elif min == 'zscale': | ||
| interval_type = 'zscale' | ||
|
|
There was a problem hiding this comment.
You need an else clause to set interval_type = None (otherwise display.scale("log", 0, 100) generates an exception)
| if unit == 'absolute' and interval_type is None: | ||
| interval_type = 'absolute' | ||
| if unit == 'percent': | ||
| interval_type = 'percent' |
There was a problem hiding this comment.
Can you use elif here? It'd be clearer
There was a problem hiding this comment.
Yes, will do. These are all mutually exclusive.
|
Post-review fixes:
|
The Firefly backend for afwDisplay requires the {{firefly_client}} module, installable via {{pip install firefly_client}}. A Firefly server must be accessible.