Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw
-
Labels:None
-
Story Points:6
-
Epic Link:
-
Sprint:Science Pipelines DM-S15-3
-
Team:Data Release Production
Description
RFC-42 (provide a backend-agnostic interface to displays) being accepted, please implement it.
For now, provide compatibility code so that the old way (import lsst.afw.display.ds9) still works.
Attachments
Issue Links
- is triggered by
-
- Implemented
Activity
Overall:
I am unhappy about code that uses afwDisplay being expected to explicitly specify a backend. I understand that it may be necessary for some very specialized code, but I hope it would be rare to need this. On the whole the interface ought to be sufficiently backend-agnostic that most code that uses afwDisplay will work with any backend. Then in most cases:
- A user can usually use his/her favorite backend
- A user need not install all supported backends
- If we ever find a newer, better display package we can transition to it as a default without rewriting very much code that uses afwDisplay
To make this work would require two things, in my opinion:
- Some way for users to specify a desired default backend, e.g. a config file
- A backup default hard-coded in afwDisplay
I am worried that the "virtualDisplay" will cause problems by silently swallowing attempts to display. Is it necessary to support this backend (except perhaps for test purposes, in which case perhaps the test code could add the display, instead of having it baked into afwDisplay?).
The treatment of frames worries me as not being sufficiently generic. I suggest the following, which is a bit like matplotlib:
- The user starts out by writing to the default frame
- If the user wants to write to a new frame, create the frame using addFrame()
- The user can display on any frame that has been created
The simplest way to identify created frames would be using indices. But other possibilities include: - Optional names. createFrame(name=None). Display keeps a dictionary of frames; the backend need not support named frames. The user can specify a frame by name or integer index.
- Objects (like matplotlib axes). A bit fussier, but more object-oriented.
Points about specific files:
Some lines in imageDisplay.dox are longer than 110 chars.
cameraGeom/utils.py:
- Shouldn't these functions take a display object? It could default to None, in which case use the default display.
- Many lines are longer than 110 cars. I realize this is a pre-existing problem, but could you please fix it?
display/ds9.py:
- Your \brief comment was changed to eliminate "ds9", but this module is explicitly about ds9, so I think that was a step backwards.
- Please add "from _future_ import absolute_import, division, print_function"
- Testing for "loaded" in globals() might be a bit clearer than using try/except, e.g.:
if not globals().get("loaded", False):if getDefaultBackend() == "virtualDevice":try:setDefaultBackend("lsst.display.ds9")except ImportError as e:raise ImportError("Unable to import lsst.display.ds9: %s" % e)loaded = True - Instead of using one technique to import Buffering from afwDisplay and a different technique to import colors, it would be less confusing to import both the same way, e.g.:
from lsst.afw.display import Buffering, BLACK, RED, GREEN, BLUE, CYAN, MAGENTA, YELLOW, WHITE
display/interface.py
- I am a bit unhappy about all those globals (_defaultBackend, _defaultFrame, _defaultMaskTransparency and _displays). You have a Display object. Wouldn't it be clearer and less messy to turn those globals into class variables? I find the present code an uncomfortable mix of object-orientation and non-object-oriented programming. If it is too annoying to have to type Display.foo instead of foo to access some of the globals, you could offer aliases, I guess (e.g. foo = Display.foo).
- For any globals you do retain, please always specify them using "global" even if you are just reading them. At present you only do this when writing, not reading. That makes the code harder to understand and makes pyflakes linter unhappy. It's also safer because otherwise if the code is altered in the future you may end up writing an unwanted local copy.
- Please define an _all_ in this module, to avoid importing extra unwanted symbols (os, sys, etc.) into the lsst.afw.display namespace.
- You import os, math and time but don't use any of them. (Also, please import one module imported per line.)
display/utils.py
- utils.getDisplay API is very confusing to me. The name matches an existing function afwDisplay.getDisplay but the API is totally different. What is the point of the utils version? If I have a display object already, why do I need to call "getDisplay" just to get it back again (after checking that the frames match)? I did not find the provided documentation to be of any help in answering these questions. I strongly suggest you delete this function. If you really do need it then please rename it and document it better. If it's purely there for backwards compatibility then I think it would be better to ditch it and change existing code.
- Please replace "from _future_ import with_statement" with "from _future_ import absolute_import, division, print_function".
- Please define _all_
- Does it make sense to import Mosaic into the lsst.afw.display namespace?
I don't really understand your unhappiness. You can configure the display system by putting something like
import lsst.afw.display as afwDisplay
|
afwDisplay.setDefaultBackend("ds9")
|
in your pythonstartup file, then
display = afwDisplay.getDisplay()
|
this seems essentially the same as the matplotlib approach, except that the verb is "get" not "add".
I deliberately did not invent a configuration system using config files and environment variables (but see my query on hipchat).
I don't think it would be appropriate to include a "real" device in afw – what would you use? Surely not ds9. I suppose we could write a matplotlib backend; but that would be a separate issue.
| display/ds9.py: |
Testing for "loaded" in globals() might be a bit clearer than using try/except, e.g.:
|
I think it's clearer and more pythonic to use try/except here
| Instead of using one technique to import Buffering from afwDisplay and a different technique to import colors, it would be less confusing to import both the same way, e.g.: |
| from lsst.afw.display import Buffering, BLACK, RED, GREEN, BLUE, CYAN, MAGENTA, YELLOW, WHITE |
This won't work, as I'm importing Buffering from the class Display
| display/interface.py |
| I am a bit unhappy about all those globals (_defaultBackend, _defaultFrame, _defaultMaskTransparency and _displays). You have a Display object. Wouldn't it be clearer and less messy to turn those globals into class variables? |
I agree; done
| display/utils.py |
| utils.getDisplay API is very confusing to me. The name matches an existing function afwDisplay.getDisplay but the API is totally different. What is the point of the utils version? |
I changed the name to make it clearer that this is a utility function to handle being given both frame and
display arguments.
| Does it make sense to import Mosaic into the lsst.afw.display namespace? |
I needed to avoid a circular import to make this work, but done.
I guess your suggestion for using a python startup file will work if the import is wrapped in try/except (since afw might not be setup). Could you please add this as a recommendation in the main.dox file for afw.display? Config files are a more common approach for this, but I understand your reluctances to open that can of worms. I felt that the response on hipchat to use a pex_config override file seemed acceptable, but that still leaves the question of what to call such files and where to put them.
Thank you for renaming utils/getDisplay to _getDisplayFromDisplayOrFrame. However, I would still like you to rewrite the documentation for _getDisplayFromDisplayOrFrame. What does it do and why is it useful? The current documentation is not only too sparse to understand, but it is also incorrect and misleading. It says "If display is None, return None..." but that is simply wrong. The function only returns None if both arguments are None. The emphasis on "either" in the brief description implies only should only specify display or frame, not both, but it's coded to allow both. Why doesn't display default to None?
Thank you for switching from global variables in display/interface.py. I'm sorry all those free functions are still needed; I was hoping they could be class methods on Display. Unfortunately pyflakes linter found several bugs in that module:
- line 302: self is unknown; I think "def flush()" should be "def flush(self)"
- lines 391 and 410: DisplayError is unknown
- line 470: the symbol "callbacks" is unknown
I have not run the linter on the rest of the code, but hope you will be willing to do so. I have configured my editor to automatically show all linter errors as I type, and to whine if I save a file with errors. The latter is occasionally annoying, but on the whole I've found it very useful.
I realized that I missed the main point of "my unhappiness" with the config system. I think display code should never specify the back end unless it is absolutely necessary (e.g. it is so fancy that it only works with one particular backend). I made a big point of this in my initial review.
Thus I think afw needs to default to something reasonable. We cannot require users to make a config file of any kind (python startup or otherwise) to use afw display!
At the time I wrote my original review your examples showed the code explicitly setting a back end. I hope you have changed that. I confess I did not check if you had.
In other words: the way in which users configure a default back end is almost irrelevant. The important point is to offer a good default and to actively discourage coders from specifying a back end in display code they write.
| I realized that I missed the main point of "my unhappiness" with the config system. I think display code should never specify the back end unless it is absolutely necessary (e.g. it is so fancy that it only works with one particular backend). I made a big point of this in my initial review. Thus I think afw needs to default to something reasonable. We cannot require users to make a config file of any kind (python startup or otherwise) to use afw display! At the time I wrote my original review your examples showed the code explicitly setting a back end. I hope you have changed that. I confess I did not check if you had. In other words: the way in which users configure a default back end is almost irrelevant. The important point is to offer a good default and to actively discourage coders from specifying a back end in display code they write. |
I though that it was pretty clear that you'd set the default backend once and not put it in scripts, but I added an explicit comment to the example.
I didn't find _getDisplayFromDisplayOrFrame hard to understand, and the comment is correct if you think about it properly; but I clarified it.
I made ds9 the default backend if it can be imported. This isn't really a long term solution (as we'd like a way to define the default default backend), but maybe it'll make people happy in the shortish term.
I think I'd rather leave those free functions (permitting the user to say afwDisplay.setDefaultBackend() rather than afwDisplay.Device.setDefaultBackend()). I've added the static methods too.
I ran pyflakes and fixed the problems it found (but not things like ds9.RED not being used in the backward-compatible afw.display.ds9 interface)
Robert Lupton could you provide an estimate of how many story points you spent on this, please?
Pushed to github. There's a test script, tests/display.py, that uses the virtual device by default. If you clone and setup git@github.com:lsst/display_ds9 you can say
tests/display.py ds9to run the tests on ds9.