DM-16704: Add fgcmcal-tract mode to fgcmcal #16

erykoff · 2019-08-26T21:47:10Z

Add new task to run fgcm on a single tract, including new convergence criteria when run in this mode.

This entails a significant refactoring of methods which were formerly private to make them accessible in the new "utilities.py" file.

Units of output reference catalog are now nJy, as per RFC-549 and RFC-575.

Also fixed bug where atmosphere wasn't being fit.

erykoff · 2019-08-26T22:01:02Z

Note that much of the code that changed was moving previous single-use methods into utilities.py so that they could be shared with the new fgcmCalibrateTract command-line task.

isullivan

Looks good overall. Please make sure to spell out the full package names in docstrings, even if they are imported with an abbreviation (such as import numpy as np). Also, when referencing Tasks or parameters in docstrings, it is helpful to put the names in double backticks. Both conventions are helpful for building links for our documentation.

isullivan · 2019-08-27T20:12:13Z

python/lsst/fgcmcal/fgcmBuildStars.py


        # Sort the visits for searching/indexing
        srcVisits.sort()

+        if len(srcVisits) == 0:
+            self.log.fatal("Found 0 visits to calibrate")
+            raise RuntimeError("No visits to calibrate")


Aren't these two errors and messages redundant?

I don't think that log.fatal raises an exception to kick out, though.

isullivan · 2019-08-27T20:13:20Z

python/lsst/fgcmcal/fgcmBuildStars.py

+        butler: `lsst.daf.persistence.Butler`
+        nCcd: `int`
+           Number of CCDs in the camera
+        dataRefs: `list` of `lsst.dat.persistence.ButlerDataRef`


typo: "dat"

isullivan · 2019-08-27T20:14:31Z

python/lsst/fgcmcal/fgcmBuildStars.py

+
+        Returns
+        -------
+        (srcVisits, srcCcds): `tuple` of `lists`s


isullivan · 2019-08-27T20:19:20Z

python/lsst/fgcmcal/fgcmBuildStars.py

+        srcCcds = []
+
+        for dataRef in dataRefs:
+            visit = dataRef.dataId[self.config.visitDataRefName]


I'm very surprised this is configurable. Are there cases where visitDataRefName is not "visit"?

DECam for example, natively uses "expnum". And in general, it could be anything.

isullivan · 2019-08-27T20:22:23Z

python/lsst/fgcmcal/fgcmBuildStars.py

+            visit = dataRef.dataId[self.config.visitDataRefName]
+
+            if visit in srcVisits:
+                continue


Doesn't this mean you will miss some srcCcds? A comment would help.

I believe you have a found a flaw in my logic, though it's the other way around. The idea is that this should be run on complete visits, but (when not run in "entire repo" mode) that should be up to the user to specify complete visits on the command line. This ignores that, and therefore bad ccds (such as HSC infamous ccd 9) would get through. I will rework the logic and make clearer comments, and will probably ask you to take a look when it's ready.

isullivan · 2019-08-27T20:44:10Z

python/lsst/fgcmcal/fgcmOutputProducts.py

+        tract: `int`
+           Tract number
+        visitCat: `lsst.afw.table.BaseCatalog`
+           FGCM visitCat from fgcmBuildStarsTask


Here and below, please put e.g. fgcmBuildStarsTask in double backticks in docstrings

isullivan · 2019-08-27T20:47:21Z

python/lsst/fgcmcal/fgcmOutputProducts.py

@@ -374,26 +426,24 @@ def _computeReferenceOffsets(self, butler):
        Parameters
        ----------
        butler: `lsst.daf.persistence.Butler`
+        FIX THIS


As the text says...

isullivan · 2019-08-27T20:48:58Z

python/lsst/fgcmcal/fgcmOutputProducts.py

        badStarKey: `afwTable.Key`
           Key for the field with bad stars
        b: `int`
           Index of the band in the star catalog
        band: `str`
           Name of band for reference catalog
-        stars: `afwTable.SimpleCatalog`
+        stdCat: `afwTable.SimpleCatalog`
           FGCM standard stars
        selected: `np.array(dtype=np.bool)`


Here and elsewhere, spell out numpy in docstrings.

isullivan · 2019-08-27T20:49:18Z

python/lsst/fgcmcal/fgcmOutputProducts.py

        stars for one pixel in one band

        Parameters
        ----------
        sourceMapper: `afwTable.SchemaMapper`
-           Mapper to go from stars to calibratable catalog
+           Mapper to go from stdCat to calibratable catalog
        badStarKey: `afwTable.Key`


Spell out lsst.afw.table.Key

isullivan · 2019-08-27T20:52:24Z

tests/fgcmcalTestBase.py

-        self.assertFloatsAlmostEqual(offsets[0], zpOffsets[0], atol=1e-6)
-        self.assertFloatsAlmostEqual(offsets[1], zpOffsets[1], atol=1e-6)
+        # self.assertFloatsAlmostEqual(offsets[0], zpOffsets[0], atol=1e-6)
+        # self.assertFloatsAlmostEqual(offsets[1], zpOffsets[1], atol=1e-6)


Delete the commented-out code.

isullivan

A couple of questions about the new code, but it looks simpler than before overall.

isullivan · 2019-08-29T21:21:12Z

python/lsst/fgcmcal/fgcmBuildStars.py

+
+        scanAllCcds = False
+        if len(dataRefs) == 0:
+            if not self.config.checkAllCcds:


I'm confused about scanAllCcds vs checkAllCcds. It looks like scanAllCcds is True only if checkAllCcds is False, but it looks like it is only checking the reference CCD in that case. Should the line scanAllCcds = True be moved to the else: statement below?

isullivan · 2019-08-29T21:23:59Z

python/lsst/fgcmcal/fgcmBuildStars.py

+                continue
+            # If we need to scan all ccds, do it here
+            if scanAllCcds:
+                dataId = dataRef.dataId


Do you need to make a copy of dataRef.dataId? I worry that you're changing elements of dataId below, and that could cause bugs later if dataRef is also changing.

Good catch.

Also overhauled the way dataRefs are chosen to ensure consistency.

erykoff added 5 commits August 26, 2019 08:05

Add first version of tract calibration code.

3267c15

This entails a significant refactoring of methods which were formerly private to make them accessible in the new "utilities.py" file.

Change reference catalog output to version 1.

99a382d

Units of output reference catalog are now nJy, as per RFC-549 and RFC-575.

Add tests for fgcmCalibrateTract and fix associated bugs.

bff6fdc

Add support for computing photometric cuts with star repeatability.

527524b

Also fixed bug where atmosphere wasn't being fit.

Add flag to tell fgcm not to create any output directories.

3361fba

isullivan approved these changes Aug 27, 2019

View reviewed changes

isullivan approved these changes Aug 29, 2019

View reviewed changes

Post-review cleanup.

671949b

Also overhauled the way dataRefs are chosen to ensure consistency.

erykoff force-pushed the tickets/DM-16704 branch from 33c027e to 671949b Compare August 29, 2019 21:57

erykoff merged commit fa3b3a3 into master Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-16704: Add fgcmcal-tract mode to fgcmcal #16

DM-16704: Add fgcmcal-tract mode to fgcmcal #16

erykoff commented Aug 26, 2019

erykoff commented Aug 26, 2019

isullivan left a comment

isullivan Aug 27, 2019

erykoff Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

erykoff Aug 27, 2019

isullivan Aug 27, 2019

erykoff Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

isullivan Aug 27, 2019

isullivan left a comment

isullivan Aug 29, 2019

isullivan Aug 29, 2019

erykoff Aug 29, 2019

DM-16704: Add fgcmcal-tract mode to fgcmcal #16

DM-16704: Add fgcmcal-tract mode to fgcmcal #16

Conversation

erykoff commented Aug 26, 2019

erykoff commented Aug 26, 2019

isullivan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isullivan left a 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