Tickets/DM-42228: add donut quality check #222

suberlak · 2024-03-14T20:44:45Z

No description provided.

jbkalmbach · 2024-03-20T00:07:42Z

doc/versionHistory.rst

@@ -14,6 +22,8 @@ Version History

 * Added unit test directly comparing ``ImageMapper`` optical models to Batoid raytracing.

+=======


This should be removed, possibly this is left over from a merge conflict?

jbkalmbach · 2024-03-20T19:58:57Z

python/lsst/ts/wep/task/estimateZernikesBase.py

@@ -153,12 +153,23 @@ def run(
            units=self.config.units,
            saveHistory=self.config.saveHistory,
        )
+        # Exclude stamps that are not effective


Can you create a config and make this an optional step? It would be easier for debugging things in the future if we can easily turn this on and off when running the pipeline.

jbkalmbach · 2024-03-20T20:02:33Z

python/lsst/ts/wep/task/estimateZernikesBase.py


        # Loop over donut stamps and estimate Zernikes
        zkList = []
        histories = dict()
        for i, (donutExtra, donutIntra) in enumerate(
-            zip(donutStampsExtra, donutStampsIntra)
+            zip(donutStampsExtraSelect, donutStampsIntraSelect)


How do we keep track of which Zernike estimates go with which donut in the end? I think we discussed adding in flags to the CalcZernikesTask to identify?

jbkalmbach · 2024-03-20T20:03:29Z

python/lsst/ts/wep/task/donutStamp.py

@@ -174,6 +175,14 @@ def factory(cls, stamp_im, metadata, index, archive_element=None):
                if metadata.get("BANDPASS") is not None
                else ""
            ),
+            # "EFFECTIVE" is a measure of entropy-based information content


Can you add something here to indicate this is a signal-to-noise effectiveness measure? Otherwise it's pretty vague.

jbkalmbach · 2024-03-20T20:03:51Z

python/lsst/ts/wep/task/donutStamps.py

@@ -179,6 +181,17 @@ def getBandpasses(self):
        """
        return [stamp.bandpass for stamp in self]

+    def getEffectives(self):
+        """
+        Get the effectiveness measure for each stamp.


Also can you add something indicating this is related to SNR?

jbkalmbach

I don't think it needs any really extensive changes. Marking as request changes because there's a lot more tests I'd like to see added and also the existing tests don't seem to be passing on Jenkins.

jbkalmbach · 2024-05-17T19:39:22Z

python/lsst/ts/wep/donutImageCheck.py

+            eff = False
+
+        # Return the actual entropy value if needed
+        if self.returnEntro:


Can you add a new test to check this works correctly?

jbkalmbach · 2024-05-17T19:40:54Z

python/lsst/ts/wep/task/calcZernikesTask.py

+        default=5000,
+        doc=str(
+            "The S/N threshold to use (keep donuts only above the threshold)."
+            + "The default S/N = 5000 corresponds to the default entropy threshold (3.5)"


I don't understand why S/N = 5000 is necessarily equivalent to an entropy of 3.5. Where does this come from?

jbkalmbach · 2024-05-17T19:43:31Z

python/lsst/ts/wep/task/calcZernikesTask.py

+
+    selectWithSignalToNoise = pexConfig.Field(
+        dtype=bool,
+        default=False,


If both selectWithSingalToNoise and selectWithEntropy are both True what is the functionality when it passes only one test? Is it logical and or does one take priority? Please add some more documentation explaining what happens in that situation as well as adding tests to verify that the expected behavior is what you see.

jbkalmbach · 2024-05-17T19:44:22Z

python/lsst/ts/wep/task/calcZernikesTask.py

@@ -120,13 +165,75 @@ def run(
            return pipeBase.Struct(
                outputZernikesRaw=np.full(19, np.nan),
                outputZernikesAvg=np.full(19, np.nan),
+                donutsExtraQuality=[],


I think these should be empty dataframes. You should add a test that checks the pipeline runs with empty content because I think this should fail since it doesn't match the output described in the connections class.

jbkalmbach · 2024-05-17T19:50:20Z

python/lsst/ts/wep/task/calcZernikesTask.py

            )
+        # Which donuts to use for Zernike estimation


This logic (down to line 226) should be moved to its own function. This needs tests as well and moving it to its own function will make it simpler to test.

jbkalmbach · 2024-05-17T22:17:57Z

python/lsst/ts/wep/task/cutOutDonutsBase.py

@@ -329,6 +427,21 @@ def cutOutStamps(self, exposure, donutCatalog, defocalType, cameraName):
        xCornerList = list()
        yCornerList = list()

+        # Centroid shift
+        drCentroidList = list()


Change to separate dx, dy as mentioned in today's AOS meeting.

jbkalmbach · 2024-05-17T22:19:22Z

python/lsst/ts/wep/task/cutOutDonutsBase.py

@@ -435,14 +566,65 @@ def cutOutStamps(self, exposure, donutCatalog, defocalType, cameraName):
        stampsMetadata["DFC_DIST"] = np.array(
            [instrument.defocalOffset * 1e3] * catalogLength
        )
+        # Save the donut flux as magnitude


What happens when you have this branch set up and you try to load butler datasets with old donut stamps? Do you run into errors?

jbkalmbach · 2024-05-17T22:24:04Z

python/lsst/ts/wep/task/cutOutDonutsCwfsTask.py

@@ -159,32 +161,25 @@ def run(
        camera: lsst.afw.cameraGeom.Camera,
    ) -> pipeBase.Struct:
        cameraName = camera.getName()
+        # The order of donut catalogs is the same


I think these changes should be rolled back. When running interactively a user might not know that they should put in extra, intra in order. Maybe what we should do is make an issue to find a way to force the donutCatalogs to have the same test as the exposures?

Could be as simple as including the detector name in our donut catalogs.

jbkalmbach · 2024-05-17T22:39:53Z

tests/task/test_calcZernikesTieTaskScienceSensor.py

+            "SN_SELECT",
+            "FINAL_SELECT",
+        ]
+        np.testing.assert_array_equal(np.sort(colnames), np.sort(desired_colnames))


As mentioned above we should test the actual logic inside the selection code.

jbkalmbach · 2024-05-17T22:46:31Z

tests/task/test_cutOutDonutsBase.py

+        # test the calculation of SN
+        sn_values = [25.88952737640593, 39.90181466115093, 33.6410205949]
+        sn_calculated = donutStamps.metadata.getArray("SN")
+        np.testing.assert_array_almost_equal(sn_values, sn_calculated, decimal=4)


Can you add a test that runs the pipeline and verifies all the metadata gets stored in the butler and when you pull the donut stamp from the repository that it still has all the metadata you added? I wonder if because the DonutStamps class doesn't know about these if they get loaded correctly.

suberlak force-pushed the tickets/DM-42228 branch from dfcc092 to 4be357d Compare March 14, 2024 20:48

suberlak requested a review from jbkalmbach March 14, 2024 20:48

suberlak force-pushed the tickets/DM-42228 branch from c1dc224 to e29c95a Compare March 19, 2024 23:49

jbkalmbach reviewed Mar 20, 2024

View reviewed changes

suberlak force-pushed the tickets/DM-42228 branch from e29c95a to 98733b1 Compare May 16, 2024 23:10

suberlak added 11 commits May 16, 2024 23:47

update version history

905c856

add donut quality check to calcZernikesTask

8999da8

add calculation of SN to cutOutDonutsBase

95c3391

make cutOutDonutsCwfsTask analogous to scienceSensor

9eaf79e

update test for cutOutDonutsBase

04721d5

remove effective from DonutStamp init in cutOutDonutsBase

a4374ef

return calculated entropy value in donutImageCheck

1903afb

update test for calcZernikesTieTask

e74ecc9

update test for calcZernikesTieTask

c9c7713

fixes to SN code in cutOutDonutsBase

951142f

updates to SN code in calcZernikesTask

7143ce2

suberlak force-pushed the tickets/DM-42228 branch from 98733b1 to 7143ce2 Compare May 16, 2024 23:47

suberlak added 2 commits May 16, 2024 23:53

isort fix to cutOutDonutsBase

e74759d

ruff fix to cutOutDonutsCwfs

4a54a33

suberlak requested a review from jbkalmbach May 16, 2024 23:54

black fixes for cutOutDonutsBase

e1deebc

jbkalmbach requested changes May 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/DM-42228: add donut quality check #222

Tickets/DM-42228: add donut quality check #222

suberlak commented Mar 14, 2024

jbkalmbach Mar 20, 2024

jbkalmbach Mar 20, 2024

jbkalmbach Mar 20, 2024

jbkalmbach Mar 20, 2024

jbkalmbach Mar 20, 2024

jbkalmbach left a comment

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

jbkalmbach May 17, 2024

		@@ -14,6 +22,8 @@ Version History

		* Added unit test directly comparing ``ImageMapper`` optical models to Batoid raytracing.

		=======

Tickets/DM-42228: add donut quality check #222

Are you sure you want to change the base?

Tickets/DM-42228: add donut quality check #222

Conversation

suberlak commented Mar 14, 2024

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

jbkalmbach 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