Add lsstSim PA1 test and serious bugfix tickets/DM-8551 #24
Conversation
parejkoj
force-pushed
the
tickets/DM-8551
branch
from
1bdf37d
to
1db7684
Compare
| self._testJointCalTask(2, relative_error, absolute_error) | ||
| dist_rms_relative = 9.7e-3*u.arcsecond | ||
| pa1 = 2.64e-3 | ||
| self._testJointCalTask(2, dist_rms_relative, dist_rms_absolute, pa1) |
There was a problem hiding this comment.
This is mostly my personal preference, but here and below please avoid "magic numbers". I'd like to see something like n_visits = 2 and then self._testJointCalTask(n_visits,...)
There was a problem hiding this comment.
In general, I agree, but I think the intention of these magic numbers is pretty clear from the context of the method name.
There was a problem hiding this comment.
Mostly true, though at least in the test above (testJointCalTask_1_visits), I really don't know how those numbers are supposed to do what the test name says.
There was a problem hiding this comment.
Because that was a bug! Thanks for catching it.
There was a problem hiding this comment.
(although it's irrelevant, given the fact that I can't run jointcal on a single catalog at present anyway.)
| # NOTE: Thus, because I'm doing a many-1 match in _make_match_dict, | ||
| # the number of matches (and thus the details of the match statistics) | ||
| # will change if the data_refs are ordered differently. | ||
| # All the more reason to use a proper n-way matcher here... |
There was a problem hiding this comment.
Is there an existing ticket you can reference here, for the n-way matcher?
There was a problem hiding this comment.
I only have the epic for reworking it (which I've added a reference to): I don't see any tickets about actually writing an n-way matcher.
| @@ -124,9 +130,10 @@ def rms_total(data): | |||
| self.old_dist_total = MatchDict(*(tuple(map(rms_total, self.old_dist))*u.radian).to(u.arcsecond)) | |||
| self.new_dist_total = MatchDict(*(tuple(map(rms_total, self.new_dist))*u.radian).to(u.arcsecond)) | |||
|
|
|||
| return self.new_dist_total.relative, self.new_dist_total.absolute | |||
| Rms_result = collections.namedtuple("rms_result", ["dist_relative", "dist_absolute", "pa1"]) | |||
There was a problem hiding this comment.
A namedtuple is like a "mini-class", so should be capitalized (see the example in the docs). That said, I should have called it "Rms_result" in the namedtuple constructor, which I have fixed.
| @@ -183,7 +188,7 @@ def rms_per_source(data): | |||
| self.old_dist_total.relative, self.old_dist_total.absolute, | |||
| self.new_dist_total.relative, self.new_dist_total.absolute, name, outdir=outdir) | |||
|
|
|||
| plot_all_wcs_deltas(plt, data_refs, visit_catalogs, old_wcs_list, name, | |||
| plot_all_wcs_deltas(plt, data_refs, self.visits_per_dataRef, old_wcs_list, name, | |||
| outdir=outdir, per_ccd_plot=per_ccd_plot) | |||
|
|
|||
| if interactive: | |||
There was a problem hiding this comment.
Not part of your changes, but do you really want this debugging code left in?
There was a problem hiding this comment.
Yes: the goal is for interactive mucking with the plots and poking at the data, which only happens if the interactive flag is set (defaults to false).
| @@ -361,12 +365,12 @@ def plot_flux_distributions(plt, old_mag, new_mag, old_jitter, new_jitter, | |||
| plt.savefig(filename.format(name)) | |||
|
|
|||
|
|
|||
| def plot_all_wcs_deltas(plt, data_refs, visit_catalogs, old_wcs_list, name, | |||
| def plot_all_wcs_deltas(plt, data_refs, visits, old_wcs_list, name, | |||
There was a problem hiding this comment.
Here and in the functions below, please define the parameters and return values.
There was a problem hiding this comment.
Done. AutoDocstring is the best.
| @@ -74,12 +75,15 @@ def _prep_reference_loader(self, center, radius): | |||
| # Make a copy of the reference catalog for in-memory contiguity. | |||
| self.reference = refLoader.loadSkyCircle(center, radius, filterName='r').refCat.copy() | |||
|
|
|||
| def _testJointCalTask(self, nCatalogs, relative_error, absolute_error): | |||
| """Test parseAndRun for jointcal on nCatalogs, requiring less than some error (arcsec).""" | |||
| def _testJointCalTask(self, nCatalogs, dist_rms_relative, dist_rms_absolute, pa1): | |||
There was a problem hiding this comment.
Please define the parameters and return value.
There was a problem hiding this comment.
Thanks: this one definitely needed a better docstring (and this gave me a reason to search for, and find, the sublimeText AutoDocstring plugin!).
|
I've updated based on your comments. Please see if the new docstrings in the latest commit are ok (some of them feel redundant with the arg names, but I wasn't sure what else to say). |
|
The new docstrings add a lot of verbosity, but look helpful for the
hypothetical frustrated developer having to dig into jointcal to find out
why something is breaking.
…On Fri, Jan 13, 2017 at 1:54 PM, John Parejko ***@***.***> wrote:
I've updated based on your comments. Please see if the new docstrings in
the latest commit are ok (some of them feel redundant with the arg names,
but I wasn't sure what else to say).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8uc0ok7ExX6f-ZAVG4m3fxz6c7Qq9kks5rR_J4gaJpZM4LicDf>
.
|
Fixed a serious bug in how I was tracking visits in JointcalStatistics, as well as a few less serious but not-trivial related bugs. In the process, cleaned up the args that various methods take (removing ones that should just be computed and handled internally) and renamed args in the plotting functions. Added pa1 test to test_jointcal_lsstSim.py, using a value just above the current computed value. Several other docstring and comment cleanups. Tweaked default plot_jointcal directory and mkdir if it's missing.
parejkoj
force-pushed
the
tickets/DM-8551
branch
from
39efece
to
16dc32d
Compare
While writing the PA1 test for lsstSim, I discovered a bug that changed the results depending on whether it was run via unittest or from plot_jointcal_results.py. The photometric part of that is fixed (the astrometric part is a "feature" of data_ref ordering which is now noted, but probably not worth fixing).