DM-39622: Add plotting for images. #59
Conversation
madamow
commented
Aug 21, 2023
madamow
commented
- plot data from different objects (Image, MaskedImage, Exposure, numpy.array)
- add centroids from list, sourceCatalog, FootprintSet
- add compass and/or grid to plot
- options to select the stretch.
python/lsst/summit/utils/plotting.py
Outdated
| import astropy.visualization as vis | ||
|
|
||
|
|
||
| def makePlot(inData, imageType='image', ax=None, centroids=None, compass=False, coordGrid=False, |
There was a problem hiding this comment.
makePlot should be renamed something like plotImage or plotExposure or perhaps just plot, given the variety of objects is accepts. Basically just remove the make and add anything appropriate/nothing at all, as make doesn't really add anything here - the relevant verb for what it's doing is plotting, so I think that should lead.
There was a problem hiding this comment.
Please remove the imageType arg entirely - it doesn't do anything, and is therefore quite confusing.
There was a problem hiding this comment.
I think the ax arg needs to change to take a figure instead, as axes are much harder to work with and less generic, so make this less extensible.
There was a problem hiding this comment.
The compass option isn't taking a compass object, it's a boolean option, so it needs a very. Something like plotCompass or showCompass or something like that to show it's a toggleable option.
There was a problem hiding this comment.
I think we need an option to add a title here too.
| stretch=None, percentile=99., cmap='gray_r', anchorPix=250, | ||
| legend=True, savePlotAs=None): | ||
|
|
||
| """Make a plot. |
There was a problem hiding this comment.
This needs to be significantly more descriptive, describing what the function does in some detail, e.g. that it supports many different types of images, shows compasses if the image has a wcs, etc.
| If input data is an exposure, plot either 'image', or 'masked' image. | ||
| Defaults to 'image'. | ||
| ax : `matplotlib.axes.Axes`, optional | ||
| The Matplotlib axis containing the image data plot. |
There was a problem hiding this comment.
It doesn't contain it here, it will be used for plotting.
| ax.text(east[0], east[1], 'E', color=color) | ||
|
|
||
| # Add centroids | ||
| if centroids: |
There was a problem hiding this comment.
I think this whole approach to dealing with multiple types of sources to plot might not be the most advisable. I think it might be more simple to have a footprints arg which accepts either a list of footprints or a footprintSet, a centroid arg which takes a list of centroids, and a sourceCat arg which accepts sourceCatalogs. I think that would make it easier for users, and make this code a little less contrived and delicate.
There was a problem hiding this comment.
I will rewrite this part of the code. I did not like it from the beginning.
|
Could you maybe file a new PR once this is ready for re-review? I'm not sure when it should be considered overhauled, and as it stands I think there might be too many comments on this one to make iteration practical. |
|
Closing this pull request. I will open a new one once all suggested changes to code are applied. |
