DM-25450: Add Rowe statistics to SQuaSH #60

arunkannawadi · 2020-08-18T20:52:31Z

This PR covers the calculation of several metrics related to Rho Statistics and their inclusion in the verify framework. There is an associated PR in lsst/verify_metrics that includes the definition of these new metrics and specifications.

MorganSchmitz

Only looked at the $\rho_0$ commit but that looks good. Is there anywhere we actually explicitely define $\rho_0$ as the autocorrelation of size residual? If so, it would be good to link to that in one of the docstrings; if not, I think it's worth adding that there.

python/lsst/pipe/analysis/analysis.py

python/lsst/pipe/analysis/utils.py

laurenam · 2020-08-28T21:21:39Z

python/lsst/pipe/analysis/utils.py

+    -------
+    xy : `treecorr.KKCorrelation`
+       A treecorr.KKCorrelation object containing the correlation function
+    """


Here and throughout, all descriptions above need backticks for referenced parameters and should end with a period. It would be nice to include valid values for ra/decUnits (and a check on them to provide a useful error message, unless treecore can be trusted to provide that).

Value checking is done by treecorr and is actually less strict than it appears to be (e.g., 'deg', 'degrees' etc. are all acceptable values by treecorr)

Sounds good, but please use double ticks for strings throughout to be consistent.

You mean double quotes for strings? I believe double ticks is for internal parameters.

laurenam · 2020-08-30T21:22:36Z

python/lsst/pipe/analysis/analysis.py

@@ -1161,6 +1165,29 @@ def plotRhoStatistics(self, description, plotInfoDict, log, treecorrParams, stat
            rhoStats = rhoStatsFunc(good_catalog)
            plotRhoStats(axes, rhoStats)

+            if verifyJob is not None:
+                verifyJob.meta.update({"shapeAlgorithm": "HSM"})


Since you are adding this to the metadata, I think you should leave the "HSM" out of the metric name so that it is not misleading if you were to compute the same metric but using a different shape algorithm.

Also, for consistency, you could use the updateVerifyJob(job, metaDict=None, specsList=None) utility function in utils.py to do the updating here.

As mentioned here, the HSM name should probably stay.

python/lsst/pipe/analysis/coaddAnalysis.py

python/lsst/pipe/analysis/utils.py

python/lsst/pipe/analysis/coaddAnalysis.py

laurenam · 2020-09-01T20:47:57Z

python/lsst/pipe/analysis/coaddAnalysis.py

@@ -516,6 +517,13 @@ def runDataRef(self, patchRefList, subdir="", cosmos=None):
        self.allStats, self.allStatsHigh = savePlots(plotList, "plotCoadd", repoInfo.dataId,
                                                     repoInfo.butler, subdir=subdir)

+        metaDict = {kk: plotInfoDict[kk] for kk in ("cameraName","filter","tract","visit","patch","rerun")}


Spaces after commas in list.

Since this is a per-tract metric, it does not make sense to include "patch" or "visit" in the metadata (I suppose one could imagine adding a "patches" or "patchList" entry that lists all the patches in the dataset, but as is, you'd only get a single one listed). Also, the camera name has been referred to as "camera" (not "cameraName") in colorAnalysis.py. If you feel strongly about "cameraName", please also change it in colorAnalysis.py (and, if you do go that route, you could also add the "rerun" entry there 😉 )

Well, this can also be called in visitAnalysis and it runs through this implementation. I include it in the metaDict only if it is not None, so that only relevant info is passed whether it is called from visitAnalysis or coaddAnalysis. And I modified it to include patchList.

The plotInfoDict has a camera object and its name as cameraName. It looks like the camera object is not serializable, nor do I want it in the metadata. To avoid confusion, I preferred to keep it as cameraName. I'll change it colorAnalysis in another ticket.

You mention that visitAnalysis can call this and that's why you left in the irrelevant visit metadata, but that's not the case. visitAnalysis does it's own metaDict setting here:
https://github.com/lsst-dm/pipe_analysis/blob/master/python/lsst/pipe/analysis/visitAnalysis.py#L527-L530
(and I don't know that you can guarantee that visit will resolve to None whenever you think it should...)
And do note the camera vs. cameraName entry. If you INSIST on naming the tag cameraName, that will be anti-pattern to what is already in SQuaSH and will leave things inconsistent within pipe_analysis. I would not want to live in this state while awaiting another ticket...I'd prefer keeping things consistent and, if you feel we must make the switch to cameraName, then that should happen everywhere on this ticket or atomically on a separate ticket.

Also, I don't think the patchList is really appropriate as a tag (how would a list of ~81 patches look in the dropdown menu...and is a list there even allowed?) I think this might be more appropriate in the measExtrasDictList.

I wouldn't insist on naming it cameraName, but would have preferred it. I've changed it to call camera now. I also exclude any patch information, since it probably is run on all 81 patches by default.

laurenam · 2020-09-01T20:51:25Z

python/lsst/pipe/analysis/utils.py

+    -------
+    xy : `treecorr.KKCorrelation`
+       A treecorr.KKCorrelation object containing the correlation function
+    """


Sounds good, but please use double ticks for strings throughout to be consistent.

python/lsst/pipe/analysis/analysis.py

python/lsst/pipe/analysis/coaddAnalysis.py

python/lsst/pipe/analysis/utils.py

python/lsst/pipe/analysis/coaddAnalysis.py

arunkannawadi · 2020-09-10T23:20:37Z

python/lsst/pipe/analysis/coaddAnalysis.py

@@ -516,6 +517,13 @@ def runDataRef(self, patchRefList, subdir="", cosmos=None):
        self.allStats, self.allStatsHigh = savePlots(plotList, "plotCoadd", repoInfo.dataId,
                                                     repoInfo.butler, subdir=subdir)

+        metaDict = {kk: plotInfoDict[kk] for kk in ("cameraName","filter","tract","visit","patch","rerun")}


Well, this can also be called in visitAnalysis and it runs through this implementation. I include it in the metaDict only if it is not None, so that only relevant info is passed whether it is called from visitAnalysis or coaddAnalysis. And I modified it to include patchList.

arunkannawadi · 2020-09-10T23:23:32Z

python/lsst/pipe/analysis/coaddAnalysis.py

@@ -516,6 +517,13 @@ def runDataRef(self, patchRefList, subdir="", cosmos=None):
        self.allStats, self.allStatsHigh = savePlots(plotList, "plotCoadd", repoInfo.dataId,
                                                     repoInfo.butler, subdir=subdir)

+        metaDict = {kk: plotInfoDict[kk] for kk in ("cameraName","filter","tract","visit","patch","rerun")}


The plotInfoDict has a camera object and its name as cameraName. It looks like the camera object is not serializable, nor do I want it in the metadata. To avoid confusion, I preferred to keep it as cameraName. I'll change it colorAnalysis in another ticket.

arunkannawadi · 2020-09-14T20:36:47Z

I think I've addressed all the comments above. Apart from naming the metrics, if there are no more issues, I'll run the Travis.

To create a `Datum` instance of the `lsst_verify` package, the `quantity` argument should be associated with a unit. Since Rho Statistics are unitless, we only provide the numerical value and pass an empty string as its unit. This does not change the default behavior if `quantity` is an `astropy.units.Quantity` instance, as the `unit` argument is ignored.

python/lsst/pipe/analysis/plotUtils.py

laurenam · 2020-09-17T17:41:29Z

python/lsst/pipe/analysis/coaddAnalysis.py

+        metaDict = {kk: plotInfoDict[kk] for kk in ("cameraName", "filter", "tract", "patchList",
+                                                    "visit", "rerun")
+                    if plotInfoDict[kk] is not None}
+        self.verifyJob = updateVerifyJob(self.verifyJob, metaDict=metaDict, specsList=None)


Looks like you lost the changes here...

python/lsst/pipe/analysis/utils.py

python/lsst/pipe/analysis/plotUtils.py

python/lsst/pipe/analysis/utils.py

python/lsst/pipe/analysis/analysis.py

The various correlations functions involving PSF ellipticity and ellipticity residuals, defined by Rho 1-5 statistics, quantity how PSF modeling errors introduce additive systematics to weak lensing shear-shear correlation functions. Similarly, the correlation of PSF size residuals contribute to a multiplicative systematic to the shear-shear correlations (see Gibin et al., 2020). Hence, we define a new Rho 0 statistic to monitor PSF size residual correlations.

Rho statistics are correlation functions involving PSF sizes, ellipticities and their residuals. However, since the lsst-verify framework only tracks scalar quantities in the dashboard, we include a functionality to produce summary statistics of the Rho statistics. Currently, such metrics include taking the average value of the correlation function(s) below and above some scales (1 arcmin by default).

arunkannawadi mentioned this pull request Aug 18, 2020

DM-25450: Add Rowe statistics to SQuaSH lsst/verify_metrics#23

Merged

MorganSchmitz reviewed Aug 20, 2020

View reviewed changes

python/lsst/pipe/analysis/analysis.py Show resolved Hide resolved

arunkannawadi force-pushed the tickets/DM-25450 branch from 0aa731a to c3cbdf7 Compare August 26, 2020 18:02

arunkannawadi changed the title ~~DM-25450~~ DM-25450 Add Rowe statistics to SQuaSH Aug 27, 2020

arunkannawadi force-pushed the tickets/DM-25450 branch from c3cbdf7 to 04b6b3c Compare August 27, 2020 19:41

arunkannawadi changed the title ~~DM-25450 Add Rowe statistics to SQuaSH~~ DM-25450: Add Rowe statistics to SQuaSH Aug 27, 2020

laurenam reviewed Aug 30, 2020

View reviewed changes

arunkannawadi mentioned this pull request Aug 31, 2020

DM-25450: Add Rowe statistics to SQuaSH lsst/obs_subaru#312

Merged

arunkannawadi force-pushed the tickets/DM-25450 branch 6 times, most recently from 8460819 to b426c7e Compare September 1, 2020 16:19

laurenam reviewed Sep 1, 2020

View reviewed changes

arunkannawadi force-pushed the tickets/DM-25450 branch from b426c7e to 1db0cab Compare September 11, 2020 00:27

arunkannawadi commented Sep 11, 2020

View reviewed changes

arunkannawadi force-pushed the tickets/DM-25450 branch 2 times, most recently from c7113fa to d651b2b Compare September 14, 2020 16:10

arunkannawadi force-pushed the tickets/DM-25450 branch 2 times, most recently from 26850a7 to f4b926d Compare September 15, 2020 22:12

laurenam reviewed Sep 17, 2020

View reviewed changes

arunkannawadi added 3 commits September 17, 2020 14:04

Convert Rho statistics into a scalar metric

7a545e1

arunkannawadi force-pushed the tickets/DM-25450 branch from f4b926d to 6bade95 Compare September 17, 2020 19:05

laurenam approved these changes Sep 17, 2020

View reviewed changes

arunkannawadi merged commit 1ec8b12 into master Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-25450: Add Rowe statistics to SQuaSH #60

DM-25450: Add Rowe statistics to SQuaSH #60

arunkannawadi commented Aug 18, 2020 •

edited

MorganSchmitz left a comment

laurenam Aug 28, 2020

arunkannawadi Aug 31, 2020

laurenam Sep 1, 2020

arunkannawadi Sep 14, 2020

laurenam Aug 30, 2020

arunkannawadi Aug 31, 2020

laurenam Sep 1, 2020

arunkannawadi Sep 10, 2020

arunkannawadi Sep 10, 2020

laurenam Sep 13, 2020

arunkannawadi Sep 14, 2020

laurenam Sep 1, 2020

arunkannawadi Sep 10, 2020

arunkannawadi Sep 10, 2020

arunkannawadi commented Sep 14, 2020

laurenam Sep 17, 2020

DM-25450: Add Rowe statistics to SQuaSH #60

DM-25450: Add Rowe statistics to SQuaSH #60

Conversation

arunkannawadi commented Aug 18, 2020 • edited

MorganSchmitz 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

Choose a reason for hiding this comment

arunkannawadi commented Sep 14, 2020

Choose a reason for hiding this comment

arunkannawadi commented Aug 18, 2020 •

edited