DM-37727: Refactor and develop further the ref cat matching code #133

sr525 · 2023-07-17T20:37:40Z

No description provided.

cmsaunders

This mostly looks good to me. I tried my best to follow all the inheritances, but didn't look super closely at code that was moved but not obviously changed.

The biggest comment is about taking out the arguments to setDefaults in refCatMatchPlots.py.

cmsaunders · 2023-07-22T17:49:08Z

python/lsst/analysis/tools/atools/refCatMatchPlots.py

+    a scatter plot.
+    """
+
+    def setDefaults(self, vectorKey):


So, Nate said recently that setDefaults shouldn't have arguments and that I never should have done things this way. If it doesn't cause huge problems, could you try just setting the fields that use vectorKey in the child classes?

Reflecting more, you could do what I did in AstrometricRelativeRepeatability and have a class variable that you set in the pipeline file, then use it to set fields in a finalize function. Then you wouldn't need separate classes for all the flux types and ra and dec. Up to you--I'm not sure what the best way is.

cmsaunders · 2023-07-22T18:07:32Z

python/lsst/analysis/tools/actions/plot/scatterplotWithTwoHists.py

@@ -721,7 +721,7 @@ def _makeSideHistogram(

        y_min, y_max = ax.get_ylim()
        bins = np.linspace(y_min, y_max, 100)
-        sideHist.hist(y_all, bins=bins, color="grey", alpha=0.3, orientation="horizontal", log=True)
+        sideHist.hist(np.array(y_all), bins=bins, color="grey", alpha=0.3, orientation="horizontal", log=True)


I realize this is outside your PR, but this line made me look at where y_all is set above, and I can't see how line 716 works. In line 719, y_all is a numpy array, but in line 716, it's a string. Also, "all" is not one of the options in the documentation for self.plotTypes or self._datatypes. Maybe @taranu remembers? In any case, I think y_all should already be a numpy array if line 716 is wrong, so you shouldn't have to add this.

I think it is supposed to be any rather than all and also data[], I will test it properly.

cmsaunders · 2023-07-22T18:11:25Z

python/lsst/analysis/tools/actions/scalar/scalarActions.py

@@ -35,7 +35,12 @@ def getInputSchema(self) -> KeyedDataSchema:

    def __call__(self, data: KeyedData, **kwargs) -> Scalar:
        mask = self.getMask(**kwargs)
-        return cast(Scalar, float(np.nanmedian(cast(Vector, data[self.vectorKey.format(**kwargs)])[mask])))
+        if data[self.vectorKey.format(**kwargs)][mask] != []:


Suggested change

if data[self.vectorKey.format(**kwargs)][mask] != []:

if len(data[self.vectorKey.format(**kwargs)][mask]) != 0:

I think this would be more agnostic of the datatype.

cmsaunders · 2023-07-22T18:12:04Z

python/lsst/analysis/tools/actions/scalar/scalarActions.py

@@ -48,7 +53,12 @@ def getInputSchema(self) -> KeyedDataSchema:

    def __call__(self, data: KeyedData, **kwargs) -> Scalar:
        mask = self.getMask(**kwargs)
-        return cast(Scalar, float(np.nanmean(cast(Vector, data[self.vectorKey.format(**kwargs)])[mask])))
+        if data[self.vectorKey.format(**kwargs)][mask] != []:


Same as above

cmsaunders · 2023-07-22T18:25:39Z

python/lsst/analysis/tools/actions/vector/selectors.py

+            errCol = (
+                f"{fluxCol}"[:fluxInd] + f"{self.uncertaintySuffix.format(**kwargs)}" + f"{fluxCol}"[fluxInd:]
+            )
+            vec = cast(Vector, data[fluxCol]) / data[errCol]


Was getting rid of the cast() around data[errCol] on purpose? It's fine if so, I just don't immediately understand why.

I don't remember. I think the cast around the result should be sufficent though?

cmsaunders · 2023-07-22T19:08:31Z

python/lsst/analysis/tools/tasks/catalogMatch.py

-        default=["u", "g", "r", "i", "z", "y"],
+    filterNames = pexConfig.ListField[str](
+        doc="Physical filter names to persist downstream.",
+        default=["HSC-G", "HSC-R", "HSC-I", "HSC-Z", "HSC-Y"],


HSC specific stuff should be in obs_subaru.

cmsaunders · 2023-07-22T19:11:36Z

python/lsst/analysis/tools/tasks/catalogMatch.py

    )

-    requireProperMotion = pexConfig.Field[bool](


Can you leave this in? I'm turning this on in a ticket I'm working on. I might be able to find a workaround if it's really annoying, since I see you aren't using the meas_algorithms reference loader directly anymore.

cmsaunders · 2023-07-22T19:34:23Z

python/lsst/analysis/tools/tasks/objectTableSurveyAnalysis.py

+        cats = []
+        for h in handle:
+            cats.append(h.get(parameters={"columns": names}))
+        return vstack(cats)


Is this still KeyedData?

cmsaunders · 2023-07-22T19:35:56Z

python/lsst/analysis/tools/tasks/photometricCatalogMatch.py

+):
+    filterNames = pexConfig.ListField[str](
+        doc="Physical filter names to persist downstream.",
+        default=["HSC-G", "HSC-R", "HSC-I", "HSC-Z", "HSC-Y"],


You have this already from CatalogMatchConfig.

cmsaunders · 2023-07-22T19:37:34Z

python/lsst/analysis/tools/tasks/photometricCatalogMatch.py

+
+
+class PhotometricCatalogMatchVisitConnections(
+    CatalogMatchConnections,


Same as for AstrometricCatalogMatchVisitConnections: I don't think you want to inherit.

…the processing carries on

sr525 force-pushed the tickets/DM-37727 branch 4 times, most recently from 07d9c47 to 099dbd9 Compare July 17, 2023 22:40

cmsaunders reviewed Jul 22, 2023

View reviewed changes

sr525 force-pushed the tickets/DM-37727 branch 2 times, most recently from d0902c4 to 02732b9 Compare July 25, 2023 13:49

sr525 force-pushed the tickets/DM-37727 branch 6 times, most recently from dba3a95 to 509a840 Compare August 29, 2023 13:33

sr525 force-pushed the tickets/DM-37727 branch 4 times, most recently from 4c5c5e0 to 572091b Compare September 7, 2023 04:21

sr525 force-pushed the tickets/DM-37727 branch from b8dfa4e to a3d6c13 Compare October 17, 2023 17:05

sr525 force-pushed the tickets/DM-37727 branch 12 times, most recently from c50e3c4 to 726de71 Compare November 22, 2023 13:18

sr525 force-pushed the tickets/DM-37727 branch 5 times, most recently from 62960cd to 95a645d Compare November 29, 2023 18:59

Sophie Reed and others added 13 commits November 29, 2023 12:02

Make plotting summary plots the default option

b4b263e

Refactor and rewrite matching tasks so that they are more reusable

dd93c57

Add tasks for plotting ref matches

4fe8e56

Remove old ref match plotting code

d00ad3c

Change the visit level plot settings to make the plots more useful

104d1a8

Get rid of the vectorKey arguments in setDefaults

3917b4e

Fix tests

ac809ea

Fix inheritance of connections

c54ac14

Update tasks to use astropy tables not dataframes

d18bf3e

Make objectTableSurveryAnalysis take mulitple inputs

c6f15a0

Raise NoWorkFound errors when the matching fails so that the rest of …

8309d6f

…the processing carries on

Fix key naming bug in sizeMagnitude plots

0c2906e

Fix logging bug in histPlot

042cfa5

sr525 force-pushed the tickets/DM-37727 branch from 95a645d to 042cfa5 Compare November 29, 2023 20:02

sr525 merged commit 7d0e31e into main Nov 30, 2023
8 checks passed

sr525 deleted the tickets/DM-37727 branch November 30, 2023 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-37727: Refactor and develop further the ref cat matching code #133

DM-37727: Refactor and develop further the ref cat matching code #133

sr525 commented Jul 17, 2023

cmsaunders left a comment •

edited

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

sr525 Aug 15, 2023 •

edited

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

sr525 Aug 15, 2023

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

cmsaunders Jul 22, 2023

	if data[self.vectorKey.format(**kwargs)][mask] != []:
	if len(data[self.vectorKey.format(**kwargs)][mask]) != 0:



		class PhotometricCatalogMatchVisitConnections(
		CatalogMatchConnections,

DM-37727: Refactor and develop further the ref cat matching code #133

DM-37727: Refactor and develop further the ref cat matching code #133

Conversation

sr525 commented Jul 17, 2023

cmsaunders left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sr525 Aug 15, 2023 • 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

cmsaunders left a comment •

edited

sr525 Aug 15, 2023 •

edited