DM-11395: Update testdata_jointcal refcats to new Indexed format #121

parejkoj · 2019-01-31T09:11:26Z

No description provided.

jdswinbank

Took a pass through the code; looks fine, with only trivial comments as attached. I've not actually tried running this yet though: will post on Jira when I've done so.

jdswinbank · 2019-01-31T16:49:39Z

python/lsst/jointcal/utils.py

@@ -234,6 +234,8 @@ def make_plots(self, data_refs, old_wcs_list,
                                    self.old_weighted_rms, self.new_weighted_rms,
                                    self.faint, self.bright, self.old_PA1, self.new_PA1,
                                    name=name, outdir=outdir)
+            self.log.info("Photometric accuracy (old, new): {:.2e} {:.2e}".format(self.old_PA1,
+                                                                                  self.new_PA1))


This makes me wince slightly, since you've no guarantee that self.old_PA1 or new_PA1 have actually been set when you hit this code (as the docstring says, it assumes that compute_rms() was run first). However, that assumption seems to be pervasive in this code (you'd probably already have failed on the previous line if it wasn't met), so I guess this is fine in context.

Yeah, all of JointcalStatistics is "not great". I want to replace it wholesale at some point, but I'd rather not think too hard about it right now.

jdswinbank · 2019-01-31T16:54:16Z

python/lsst/jointcal/jointcal.py

@@ -320,8 +320,12 @@ def __init__(self, butler=None, profile_jointcal=False, **kwargs):
        self.makeSubtask("sourceSelector")
        if self.config.doAstrometry:
            self.makeSubtask('astrometryRefObjLoader', butler=butler)
+        else:
+            self.astrometryRefObjLoader = None


I was slightly confused by the commit message that “we can't create the ref loader in setUp anymore” associated with this change — it sent me off looking for a setUp method in Jointcal. Maybe tweak it slightly to “in the test case's setUp method” or something?

jdswinbank · 2019-01-31T17:07:54Z

tests/test_jointcal_cfht.py

@@ -195,7 +200,8 @@ def test_jointcalTask_2_visits_constrainedPhotometry_lineSearch(self):
        self.config.allowLineSearch = True

        # Only this value should differ from the metrics defined in setup above.


“These values”.

Actually, I found this comment helpful, and wish I'd come across it earlier. It took me a moment to work out that what you were doing on each test was having the setup_jointcalTask... method populate a dict of expected metrics for a default configuration, then tweaking the configuration and the metrics to suit what you want to test in this particular method. I think that's fine, but it might be more clearly signposted.

I've clarified the setup_jointcalTask_blah docstrings, which should help?

jdswinbank · 2019-01-31T17:08:41Z

tests/test_jointcal_cfht.py

@@ -187,6 +190,8 @@ def test_jointcalTask_2_visits_constrainedPhotometry_no_rank_update(self):
        pa1, metrics = self.setup_jointcalTask_2_visits_constrainedPhotometry()
        self.config.photometryDoRankUpdate = False

+        metrics['photometry_final_chi2'] = 3297.34


So it does affect the chi2 (ref method docstring).

I've gone through and clarified various docstrings based on thinking about things and looking at logs in more detail.

jdswinbank · 2019-01-31T17:19:48Z

tests/test_jointcal_hsc.py

-                   'astrometry_final_chi2': 691.1210,
-                   'astrometry_final_ndof': 1858,
+                   'astrometry_final_chi2': 819.07,
+                   'astrometry_final_ndof': 2134,


Not directly related to the change here, but a couple of lines below — should it be obvious why turning off photometric fitting means you're also setting astrometryModel = "simple"?

The original tests all used the "simple" model: it used to be the default. The default is now "constrained" (per DM-15421). I'll update the names of these tests to make it more explicit.

jdswinbank · 2019-01-31T17:22:24Z

tests/data/cfht_minimal/ref_cats/sdss-dr9-fink-v5b/config.py

+config.ref_dataset_name='sdss'
+
+# Depth of the HTM tree to make.  Default is depth=7 which gives
+#               ~ 0.3 sq. deg. per trixel.


I'm not sure what the sense of default is here — are you saying that the value you have set in this file is 7 (in which case: yes, I know! It's written on the line below!) or that there's some default set elsewhere and that you are agreeing with it by setting the same value here?

This one is easy: I just copied these files from the validation_data_cfht repo, and I don't know anything about what they mean.

jdswinbank · 2019-01-31T17:24:21Z

tests/data/README.md

@@ -0,0 +1,2 @@
+A trivial dataset to do a minimal test of jointcal functionality, without having to download the full testdata_jointcal repo.
+This data is small enough that the final photometry chi2 can be computed a-priori by solving the matrix system directly.


Is there code available to do that? I was half expecting to see it worked out here, but ... it wasn't.

Good point: it's in a notebook that is committed. I've updated the readme to point to it.

Have jointcal return refObjLoaders and use them directly in tests: Indexed refcats need a Butler at instantiation, so we can't create the ref loader in the tests' setUp any more. We can revisit how we do this when we write a gen3 jointcal

I hadn't caught all cases of this hack when I cleaned up the test code a while back.

These SDSS indexed refcats did not have any cuts applied, so the metric values have changed, because the number of reference sources per CCD has increased. The cfht, decam, and hsc tests are all now updated to use the Indexed version of the refcats, instead of the ones built on a.net. Add independent lsstSim test config (it still needs the a.net refcat). This twinkles data will eventually be replaced by new DC2 simulations built on obs_lsst, and those will have proper Indexed refcats, so there's no point modifying that particular data.

Add readme files describing this testdata. Add one Indexed refcat trixel covering the single ccd, so that we no longer depend on testdata_jointcal at all for the cfht_minimal test.

A handful of the new reference sources are causing the baseline decam constrained astrometry test to give a different chi2 on macOS. It rejects a handful more outliers: sub-selecting the refcat manually before fitting makes macOS and Linux give an identical result. Given the oddities in the decam data (and the number of decam tests that are currently disabled), I think we're best off leaving exploration of this to DM-17597.

Now that the default model is "constrained", some of the test names weren't explicit enough about what was being tested.

Add printing of PA1 measurement for tests

e977ad3

jdswinbank approved these changes Jan 31, 2019

View reviewed changes

parejkoj force-pushed the tickets/DM-11395 branch from 5c5f78b to 1859dee Compare February 1, 2019 01:42

parejkoj added 7 commits February 1, 2019 14:23

Cleanup old version of no-photometry test

c4e03f7

I hadn't caught all cases of this hack when I cleaned up the test code a while back.

Ignore new config files in setup.cfg

04d4cae

Update cfht_minimal test to use Indexed refcat

c52a477

Add readme files describing this testdata. Add one Indexed refcat trixel covering the single ccd, so that we no longer depend on testdata_jointcal at all for the cfht_minimal test.

Cleanup hsc test names

f10ef3c

Now that the default model is "constrained", some of the test names weren't explicit enough about what was being tested.

parejkoj force-pushed the tickets/DM-11395 branch from a11ca51 to f10ef3c Compare February 1, 2019 22:29

parejkoj merged commit 44d5b4c into master Feb 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-11395: Update testdata_jointcal refcats to new Indexed format #121

DM-11395: Update testdata_jointcal refcats to new Indexed format #121

parejkoj commented Jan 31, 2019

jdswinbank left a comment

jdswinbank Jan 31, 2019

parejkoj Jan 31, 2019 •

edited

jdswinbank Jan 31, 2019

jdswinbank Jan 31, 2019

jdswinbank Jan 31, 2019

parejkoj Feb 1, 2019

jdswinbank Jan 31, 2019

parejkoj Feb 1, 2019

jdswinbank Jan 31, 2019

parejkoj Feb 1, 2019

jdswinbank Jan 31, 2019

parejkoj Jan 31, 2019

jdswinbank Jan 31, 2019

parejkoj Jan 31, 2019

		@@ -195,7 +200,8 @@ def test_jointcalTask_2_visits_constrainedPhotometry_lineSearch(self):
		self.config.allowLineSearch = True

		# Only this value should differ from the metrics defined in setup above.

		@@ -0,0 +1,2 @@
		A trivial dataset to do a minimal test of jointcal functionality, without having to download the full testdata_jointcal repo.
		This data is small enough that the final photometry chi2 can be computed a-priori by solving the matrix system directly.

DM-11395: Update testdata_jointcal refcats to new Indexed format #121

DM-11395: Update testdata_jointcal refcats to new Indexed format #121

Conversation

parejkoj commented Jan 31, 2019

jdswinbank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Jan 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Jan 31, 2019 •

edited