DM-36182: Cleanup and document functors.py. #811

leeskelvin · 2023-07-05T19:21:46Z

No description provided.

arunkannawadi · 2023-07-06T19:55:04Z

python/lsst/pipe/tasks/functors.py

@@ -778,7 +785,7 @@ def __init__(self, col, calib=None, **kwargs):
        if calib is not None:
            self.fluxMag0 = calib.getFluxMag0()[0]


If we want to remove calib, we should first document that this is an unused argument in the docstring, remove this line and instead raise a deprecation warning here. I'd follow a pattern similar to these lines: https://github.com/lsst/pex_config/blob/aa9e307f6b0b52071272d5b1404e053a96c8fdb0/python/lsst/pex/config/config.py#L1543-L1546

How does this revision (just pushed) look to you? Future deprecation ticket: DM-39914.

This looks fine, as long as you're sure getFluxMag0 exists. My GitHub search returns no implementation of it. If that's the case, I'd remove it right away.

And either way, the warning should come first before it executes the code in the if block.

As this hasn't been deprecated officially yet, people may still want to use this to access older data. I realize that this may not be something we need to support here, but regardless, my feeling was that it costs us nothing to keep it in for now just in case.

arunkannawadi · 2023-07-06T20:31:14Z

python/lsst/pipe/tasks/functors.py

            self.fluxMag0 = calib.getFluxMag0()[0]
+            warnings.warn(
+                "Config field 'calib' is deprecated, and will be removed after v27.",


I don't think Config field is applicable here. It should perhaps be "calib argument".

Changed to The 'calib' argument is deprecated, and will be removed after v27..

arunkannawadi · 2023-07-06T20:36:33Z

python/lsst/pipe/tasks/functors.py

@@ -778,7 +785,7 @@ def __init__(self, col, calib=None, **kwargs):
        if calib is not None:
            self.fluxMag0 = calib.getFluxMag0()[0]


This looks fine, as long as you're sure getFluxMag0 exists. My GitHub search returns no implementation of it. If that's the case, I'd remove it right away.

taranu · 2023-07-10T21:58:29Z

python/lsst/pipe/tasks/functors.py

+    ``deepCoadd_obj`` dataset; that is, a DataFrame with a multi-level column
+    index, with the levels of the column index being ``band``, ``dataset``, and
+    ``column``.
+    It has since been generalized to apply to DataFrames without mutli-level


multi-level

taranu

Looks fine. I have nitpicky suggestions/questions but nothing particularly important.

taranu · 2023-07-10T22:00:05Z

python/lsst/pipe/tasks/functors.py

    it gets to have those levels in the column index.

    Parameters
    ----------
    filt : str
-        Filter upon which to do the calculation
+        Filter upon which to do the calculation.


Maybe change to say it's actually the band, like the docstring says?

taranu · 2023-07-10T22:35:33Z

python/lsst/pipe/tasks/functors.py

-    calib : `lsst.afw.image.calib.Calib` (optional)
-        Object that knows zero point.
+        Name of flux column from which to compute magnitude.
+        Can be parseable by `~lsst.pipe.tasks.functors.fluxName` function; that


by the ...fluxName function?

taranu · 2023-07-10T22:44:55Z

python/lsst/pipe/tasks/functors.py

-    """Functor to calculate SDSS trace radius size for sources"""
+    """Functor to calculate SDSS trace radius size for sources.
+
+    SDSS trace radius size is a measure of size equal to the square root of


I'd prefer The SDSS trace radius size.... But slightly more importantly, I think the functor name ought to be SDSS Trace Radius, as I think radius is strictly more specific than size. I suppose changing the actual functor class name is not worth the effort.

I think renaming of functors is beyond the scope of this ticket. If you feel strongly about this though, please do set up a follow-up ticket to make this singular change.

taranu · 2023-07-10T22:45:26Z

python/lsst/pipe/tasks/functors.py

-    """Functor to calculate HSM trace radius size for sources"""
+    """Functor to calculate HSM trace radius size for sources.
+
+    HSM trace radius size is a measure of size equal to the square root of


The HSM trace radius size...

taranu · 2023-07-10T22:45:54Z

python/lsst/pipe/tasks/functors.py

@@ -974,7 +1005,13 @@ def _func(self, df):


 class PsfSdssTraceSizeDiff(Functor):
-    """Functor to calculate SDSS trace radius size difference (%) between object and psf model"""
+    """Functor to calculate SDSS trace radius size difference (%) between the


the SDSS trace...

taranu · 2023-07-10T22:46:03Z

python/lsst/pipe/tasks/functors.py

@@ -1001,7 +1044,13 @@ def _func(self, df):


 class PsfHsmTraceSizeDiff(Functor):
-    """Functor to calculate HSM trace radius size difference (%) between object and psf model"""
+    """Functor to calculate HSM trace radius size difference (%) between the


the HSM trace...

taranu · 2023-07-10T22:46:33Z

python/lsst/pipe/tasks/functors.py

@@ -1019,6 +1068,15 @@ def _func(self, df):


 class HsmFwhm(Functor):
+    """Functor to calculate PSF FWHM with second moments measured from the


the PSF FWHM using...

taranu · 2023-07-10T22:47:56Z

python/lsst/pipe/tasks/functors.py

@@ -1067,6 +1143,15 @@ def _func(self, df):


 class RadiusFromQuadrupole(Functor):
+    """Calculate the radius from the quadrupole moments.
+    This returns the fourth root of the determinant of the second moments


I think there's supposed to be a newline between this line and the one above.

taranu · 2023-07-10T22:48:35Z

python/lsst/pipe/tasks/functors.py

@@ -1067,6 +1143,15 @@ def _func(self, df):


 class RadiusFromQuadrupole(Functor):


This really should have been DeterminantRadius to be consistent with the TraceRadius usages above, but I suppose it's not worth changing at this point either.

As above, if you feel strongly, please do put this on a follow-up ticket.

taranu · 2023-07-10T23:18:50Z

python/lsst/pipe/tasks/postprocess.py

-                    - base_PsfFlux
-                filt: HSC-G
-            count:
+            sourceId:


What was the motivation for this change? The previous example was out of date, or non-functional?

The initial motivation was to replace the mis-named flags section and replace the MagDiff examples, as MagDiff was going away. On review, MagDiff remains, but the example edits here were preserved to showcase more typical examples.

leeskelvin force-pushed the tickets/DM-36182 branch 8 times, most recently from 1357711 to 2a4562c Compare July 6, 2023 16:30

arunkannawadi reviewed Jul 6, 2023

View reviewed changes

leeskelvin force-pushed the tickets/DM-36182 branch from 2a4562c to 78857ba Compare July 6, 2023 20:17

arunkannawadi reviewed Jul 6, 2023

View reviewed changes

leeskelvin force-pushed the tickets/DM-36182 branch from 78857ba to 33542b0 Compare July 6, 2023 20:53

taranu reviewed Jul 10, 2023

View reviewed changes

taranu approved these changes Jul 10, 2023

View reviewed changes

Cleanup and document functors.py.

a216322

leeskelvin force-pushed the tickets/DM-36182 branch from 33542b0 to a216322 Compare July 11, 2023 15:28

leeskelvin merged commit 36c42b5 into main Jul 11, 2023
2 checks passed

leeskelvin deleted the tickets/DM-36182 branch July 11, 2023 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-36182: Cleanup and document functors.py. #811

DM-36182: Cleanup and document functors.py. #811

leeskelvin commented Jul 5, 2023

arunkannawadi Jul 6, 2023

leeskelvin Jul 6, 2023

arunkannawadi Jul 6, 2023

arunkannawadi Jul 6, 2023

leeskelvin Jul 6, 2023

arunkannawadi Jul 6, 2023

leeskelvin Jul 6, 2023

arunkannawadi Jul 6, 2023

taranu Jul 10, 2023

taranu left a comment

taranu Jul 10, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

leeskelvin Jul 11, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

taranu Jul 10, 2023

leeskelvin Jul 11, 2023

taranu Jul 10, 2023

leeskelvin Jul 11, 2023

		@@ -778,7 +785,7 @@ def __init__(self, col, calib=None, **kwargs):
		if calib is not None:
		self.fluxMag0 = calib.getFluxMag0()[0]

		@@ -1019,6 +1068,15 @@ def _func(self, df):


		class HsmFwhm(Functor):
		"""Functor to calculate PSF FWHM with second moments measured from the

		@@ -1067,6 +1143,15 @@ def _func(self, df):


		class RadiusFromQuadrupole(Functor):

DM-36182: Cleanup and document functors.py. #811

DM-36182: Cleanup and document functors.py. #811

Conversation

leeskelvin commented Jul 5, 2023

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

taranu 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