Improve jointcal plotting backend Tickets/dm 7444 #19
Conversation
parejkoj
force-pushed
the
tickets/DM-7444
branch
from
1296356
to
04e3dc0
Compare
There was a problem hiding this comment.
Thank you very much for all of the great docstrings, it certainly made it very easy to follow your code. I know that a lot of this was copied from the test, so my apologies if I'm commenting on things that were just copied over from before.
Perhaps some of my comments are because I'm new to using the stack (as opposed to working on the framework), so some of the things I'm asking for might be obvious to most people.
The main thing I'd like to see in the docstrings is a little more information about the parameters that are lists. While in general, python lists can have any (or multiple types) of objects in them, for most of these functions the expected type is known and required, so it would be nice to see that in the docstrings. I only requested this once in a comment, but it would be nice to see this for all of the list parameters.
Once our docs are made with Sphinx it will also be useful to have the types written so that they can be linked. For example, giving the parameter type as "lsst.afw.table.SourceCatalog" instead of "catalog".
| name = os.path.split(name[0])[1] | ||
| else: | ||
| name = name[1] | ||
| return name |
There was a problem hiding this comment.
I'm not sure what this is supposed to be doing. The docstring makes it sound like the equivalent of os.path.basename, but in that case I think you need to return name[-1], not name[1], right? But really if this is correct, you could just use os.path.basename instead and remove this function.
There was a problem hiding this comment.
The problem is, basename also returns '' if the path ends in a /. But I found a much better solution, so thanks for bringing up this function: os.path.basename(os.path.normpath(args.repo))
| import lsst.afw.table | ||
|
|
||
|
|
||
| class JointcalStatistics(object): |
There was a problem hiding this comment.
A short docstring to describe the class might be nice.
| 100 is a balance between good centroids and enough sources. | ||
| """ | ||
| self.match_radius = match_radius | ||
| self.flux_limit = 100 |
There was a problem hiding this comment.
I think this should be self.flux_limit = flux_limit.
There was a problem hiding this comment.
Good catch. I guess I was dumb.
|
|
||
| Parameters | ||
| ---------- | ||
| data_refs : list |
There was a problem hiding this comment.
It might be useful to state what this is a list of, ie lsst.daf.persistence.butlerSubset.ButlerDataRef I think.
There was a problem hiding this comment.
Good idea. I cleared these up as best I could.
| Returns | ||
| ------- | ||
| dict | ||
| dict of sourceID:list(measurement deltas for that source) |
There was a problem hiding this comment.
Perhaps change measurement deltas to separation distances to make its contents more clear?
There was a problem hiding this comment.
That's clearer, thanks.
| plt.xlabel('RA') | ||
| plt.ylabel('Dec') | ||
| plt.title('visit: {}'.format(cat)) | ||
| plt.savefig('.plots/{}-{}-quivers.pdf'.format(name, cat)) |
There was a problem hiding this comment.
Should the user have the option to set the base path for the plots?
| xmin = x1.min() if x1.min() < xmin else xmin | ||
| ymin = y1.min() if y1.min() < ymin else ymin | ||
| xmax = x1.max() if x1.max() > xmax else xmax | ||
| ymax = y1.max() if y1.max() > ymax else ymax |
There was a problem hiding this comment.
I think these can be simplified using xmin = np.min([x1.min(), xmin]) and similar.
There was a problem hiding this comment.
Good idea. That's much nicer.
| plt.xlim(plotOptions['range']) | ||
| plt.xlabel('arcseconds') | ||
| plt.legend(loc='best') | ||
| plt.savefig('.plots/%s-histogram.pdf'%name) |
There was a problem hiding this comment.
It would also be nice to save this in an arbitrary path
| plt.plot((x1 - x2) + center[0], (y1 - y2) + center[1], '-') | ||
| plt.xlabel('delta RA (arcsec)') | ||
| plt.ylabel('delta Dec (arcsec)') | ||
| plt.title(name) |
There was a problem hiding this comment.
These never appear to be saved anywhere like all of the other plots. Is that intentional?
There was a problem hiding this comment.
This was a earlier plot with more limited utility, but I've added the option to save it. Since the code is there already, it can't hurt.
| help="Radius (degrees) of sources to load from reference catalog.") | ||
| parser.add_argument("-i", "--interactive", action="store_true", | ||
| help="Use interactive matplotlib backend and set ion(), in addition to saving files.") | ||
| args = parser.parse_args() |
There was a problem hiding this comment.
It may be useful here to include an argument that allows the user to specify an output directory
There was a problem hiding this comment.
Good suggestion. Done.
parejkoj
force-pushed
the
tickets/DM-7444
branch
2 times, most recently
from
d1fa818
to
ffda056
Compare
Added plotting script to make plots from jointcal runs outside of test code. Cleanup matplotlib imports so only utils cares about matplotlib.
parejkoj
force-pushed
the
tickets/DM-7444
branch
from
6be9018
to
ee82564
Compare
Moved the jointcal plotting and RMS calculation code out of the tests and into
utils.py, and added abin/script to make plots outside of test running (which will be helpful for working on plotting later). Also refactored testing base class.utils.pydoesn't have any tests, but the algorithms in it are pretty dumb, and should be replaced as part of DM-8664.