DM-33960: Add astrometry residuals with refcat plots #22

cmsaunders · 2022-03-15T16:25:28Z

No description provided.

yalsayyad · 2022-03-15T18:52:43Z

python/lsst/analysis/drp/catalogMatch.py

+__all__ = ["CatalogMatchTaskConfig", "CatalogMatchTask"]
+
+
+class AstropyMatchTaskConfig(pexConfig.Config):


If a task is called AstropyMatchTask, convention is to call the corresponding config AstropyMatchConfig. (I saw a couple places where this convention was not followed in analysis_drp so I'm guessing you just copied and pasted from there).

yalsayyad · 2022-03-15T18:53:57Z

python/lsst/analysis/drp/catalogMatch.py

+    ConfigClass = AstropyMatchTaskConfig
+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)


This __init__ doesn't do anything different than super's __init__. Whole method unnecessary.

yalsayyad · 2022-03-15T18:59:57Z

python/lsst/analysis/drp/catalogMatch.py

+
+        separations = d2d[goodMatches]
+
+        return refMatchIndices, targetMatchIndices, separations


run methods in Tasks should return a Struct.

return pipeBase.Struct(refMatchIndices=refMatchIndices, targetMatchIndices=targetMatchIndices separations=separations)

The Task frameowrk expects shit. Reasons for this is that is that devs can extend the method to return more later without breaking any calling code. Default runQuantum tries writes the keys of the Struct as output.

yalsayyad · 2022-03-15T19:03:05Z

pipelines/coaddQAPlots.yaml

+                   "y": "$Dec_{target} - Dec_{ref}$ (marcsec)"}
+      python: |
+        from lsst.analysis.drp.dataSelectors import CoaddPlotFlagSelector, StarIdentifier
+        from lsst.analysis.drp.calcFunctors import AstromDiff


needs a newline

yalsayyad · 2022-03-15T19:14:01Z

python/lsst/analysis/drp/catalogMatch.py

+                "_target" for duplicated column names, plus a column with the
+                angular separation between matches.
+        """
+


I've seen a few of violations of:

https://developer.lsst.io/python/numpydoc.html#docstrings-of-methods-and-functions-should-not-be-preceded-or-followed-by-a-blank-line

https://developer.lsst.io/python/numpydoc.html#docstrings-of-classes-should-be-followed-but-not-preceded-by-a-blank-line

Not going to comment on every one.

Argh, I can never remember which is right!

yalsayyad · 2022-03-15T19:36:50Z

python/lsst/analysis/drp/catalogMatch.py

+        # Join the catalogs for the matched catalogs
+        refMatches = self.refCat.iloc[refMatches].reset_index()
+        sourceMatches = targetCatalog.iloc[targetMatches].reset_index()
+        matchedCat = sourceMatches.join(refMatches, lsuffix='_target', rsuffix='_ref')


What's it joining on if there's no index or "on" arg?

The default for 'on' is to join on the index.

yalsayyad · 2022-03-15T19:40:27Z

python/lsst/analysis/drp/catalogMatch.py

+        self.refCat = refCat.asAstropy().to_pandas()
+
+
+class CatalogMatchVisitTaskConnections(pipeBase.PipelineTaskConnections, dimensions=("visit", "skymap"),


@natelust Out of curiosity, why we need skymap here? Is it to get the refCat shards to be found?

yalsayyad · 2022-03-15T19:42:18Z

python/lsst/analysis/drp/catalogMatch.py

+    """
+
+    ConfigClass = CatalogMatchVisitTaskConfig
+    _DefaultName = "catalogMatchTask"


convention is "catalogMatch" for the DefaultNames. lowercase. Leave off the word Task.

yalsayyad · 2022-03-15T19:52:15Z

python/lsst/analysis/drp/catalogMatch.py

+        inputs = butlerQC.get(inputRefs)
+
+        dataFrame = inputs["catalog"].get()
+        inputs['catalog'] = dataFrame


Ooof, I just realized that you're reading in the entire objectTable_tract and making a copy of the whole thing. Don't deferLoad if you're going to read in the whole thing, but I'm going to think about alternatives to this, because this not the way if we want the analysis_drp to be fast and memory efficient.

yalsayyad · 2022-03-15T19:56:29Z

python/lsst/analysis/drp/catalogMatch.py

+        # dataframe
+        skyCircle = self.refObjLoader.loadSkyCircle(center,
+                                                    radius,
+                                                    'i',


intentionally hardcoded band?

Yes, the band is ignored by a config, but you apparently have to put in something.

yalsayyad · 2022-03-16T01:55:08Z

pipelines/matchVisitCatalogs.yaml

+      selectorActions.flagSelector: VisitPlotFlagSelector
+      sourceSelectorActions.sourceSelector.band: ""
+      python: |
+        from lsst.analysis.drp.dataSelectors import VisitPlotFlagSelector


needs newline.

yalsayyad

Looks fine. This is next on the list of plots/metrics to migrate to analysis_tools as an example

yalsayyad reviewed Mar 15, 2022

View reviewed changes

cmsaunders force-pushed the tickets/DM-33960 branch from 2f1f2f1 to 35b873a Compare March 16, 2022 01:37

yalsayyad reviewed Mar 16, 2022

View reviewed changes

cmsaunders force-pushed the tickets/DM-33960 branch 3 times, most recently from 216cd9b to e3797a2 Compare March 16, 2022 19:46

cmsaunders force-pushed the tickets/DM-33960 branch 3 times, most recently from 925d0de to 73f0207 Compare April 27, 2022 19:48

yalsayyad approved these changes Jun 16, 2022

View reviewed changes

cmsaunders added 3 commits June 20, 2022 14:41

Add catalog matching task

908dd35

Add dRA and dDec plots for visits and coadds

9ca2c48

Add reference catalog match to pipelines

2b0d7e3

cmsaunders force-pushed the tickets/DM-33960 branch from 73f0207 to 2b0d7e3 Compare June 20, 2022 19:41

cmsaunders merged commit 8673edb into main Jun 20, 2022

cmsaunders deleted the tickets/DM-33960 branch June 20, 2022 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-33960: Add astrometry residuals with refcat plots #22

DM-33960: Add astrometry residuals with refcat plots #22

cmsaunders commented Mar 15, 2022

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

cmsaunders Mar 16, 2022

yalsayyad Mar 15, 2022

cmsaunders Mar 16, 2022

yalsayyad Mar 15, 2022 •

edited

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

yalsayyad Mar 15, 2022

cmsaunders Mar 16, 2022

yalsayyad Mar 16, 2022

yalsayyad left a comment

		__all__ = ["CatalogMatchTaskConfig", "CatalogMatchTask"]


		class AstropyMatchTaskConfig(pexConfig.Config):


		separations = d2d[goodMatches]

		return refMatchIndices, targetMatchIndices, separations

		self.refCat = refCat.asAstropy().to_pandas()


		class CatalogMatchVisitTaskConnections(pipeBase.PipelineTaskConnections, dimensions=("visit", "skymap"),

DM-33960: Add astrometry residuals with refcat plots #22

DM-33960: Add astrometry residuals with refcat plots #22

Conversation

cmsaunders commented Mar 15, 2022

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

yalsayyad Mar 15, 2022 • 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

yalsayyad left a comment

Choose a reason for hiding this comment

yalsayyad Mar 15, 2022 •

edited