DM-46966: Make calibrating pixel values optional in CalibrateImageTask #996

TallJimbo · 2024-10-19T13:08:43Z

No description provided.

erykoff · 2024-10-21T16:13:41Z

python/lsst/pipe/tasks/calibrateImage.py

+        default=True,
+        doc=(
+            "If True, apply the photometric calibration to the image pixels "
+            "and background model, and attach an identify PhotoCalib to "


Identity not identify

erykoff · 2024-10-21T16:15:38Z

python/lsst/pipe/tasks/calibrateImage.py

+        ----------
+        exposure : `lsst.afw.image.Exposure`
+            Exposure with the target `lsst.afw.image.PhotoCalib` attached.
+            On return, pixel values will be calibrated and an identify


parejkoj

Should setting this new config to False also require changing the exposure output name to something other than initial_pvi, so that we can keep track of which ones were calibrated and which not? That's my main worry here, since right now the only way to know is to check whether the attached PhotoCalib is ==1.

parejkoj · 2024-11-04T23:57:12Z

python/lsst/pipe/tasks/calibrateImage.py

+        doc=(
+            "If True, apply the photometric calibration to the image pixels "
+            "and background model, and attach an identity PhotoCalib to "
+            "the output image to reflect this."


Maybe add something like "Otherwise, leave the image and background uncalibrated and attach the PhotoCalib that would calibrate them."

parejkoj · 2024-11-04T23:57:41Z

python/lsst/pipe/tasks/calibrateImage.py

+
+                This is `None` if ``config.do_calibrate_pixels`` is `False`.


Is a newline allowed here? I've never done that before.

I think so, but it doesn't really do any harm to drop it, and I'll do that since apparently package-docs build is messed up somehow for me locally and I don't want to wade into that.

parejkoj · 2024-11-05T00:00:07Z

python/lsst/pipe/tasks/calibrateImage.py

+        background : `lsst.afw.math.BackgroundList`
+            Background model to convert to nanojansky units in place.


You got rid of this parameter, no?

parejkoj · 2024-11-05T00:01:10Z

python/lsst/pipe/tasks/calibrateImage.py


        Returns
        -------
        calibrated_stars : `lsst.afw.table.SourceCatalog`
            Star catalog with flux/magnitude columns computed from the fitted
-            photoCalib.
+            photoCalib (instFlux columns are retained as well).
        matches : `list` [`lsst.afw.table.ReferenceMatch`]
            Reference/stars matches used in the fit.


It looks like we need to also add matchMeta to this output list, too. Sorry I'd neglected that.

parejkoj · 2024-11-05T00:02:29Z

python/lsst/pipe/tasks/calibrateImage.py

+        background : `lsst.afw.math.BackgroundList`
+            Background model to convert to nanojansky units in place.
+        """
+        photo_calib = exposure.getPhotoCalib()


Why not use exposure.photoCalib below, instead of assigning it? Doesn't really matter, I just don't like proliferating the getX when we don't have to.

It's because I was going to need this as a local variable anyway later in the method, after exposure.photoCalib has been set to the identity.

Oh, right, good point.

parejkoj · 2024-11-05T00:04:00Z

python/lsst/pipe/tasks/calibrateImage.py

-        # TODO investigation: because this takes the photoCalib from the
-        # exposure, photometric summary values may be "incorrect" (i.e. they
-        # will reflect the ==1 nJy calibration on the exposure, not the
-        # applied calibration). This needs to be checked.


Well, at least I'd left a TODO to reflect this that can be retired!

parejkoj · 2024-11-05T00:04:27Z

tests/test_calibrateImage.py

+            self.assertEqual(result.exposure.getPhotoCalib().getCalibrationMean(), 1.0)
+        else:
+            self.assertIsNone(result.applied_photo_calib)
+            photo_calib = result.exposure.getPhotoCalib()


Suggested change

photo_calib = result.exposure.getPhotoCalib()

photo_calib = result.exposure.photoCalib

parejkoj · 2024-11-05T00:04:41Z

tests/test_calibrateImage.py

+            # Fit photoCalib should be the applied value if we calibrated
+            # pixels, not the ==1 one on the exposure.
+            photo_calib = result.applied_photo_calib
+            self.assertEqual(result.exposure.getPhotoCalib().getCalibrationMean(), 1.0)


Suggested change

self.assertEqual(result.exposure.getPhotoCalib().getCalibrationMean(), 1.0)

self.assertEqual(result.exposure.photoCalib.getCalibrationMean(), 1.0)

parejkoj · 2024-11-05T00:05:46Z

tests/test_calibrateImage.py

+        """Test that run() returns reasonable values to be butler put when
+        do_calibrate_pixels=False.


I guess this came from my original wording, but this is much clearer.

Suggested change

"""Test that run() returns reasonable values to be butler put when

do_calibrate_pixels=False.

"""Test that run() returns reasonable values when

``do_calibrate_pixels=False``.

parejkoj · 2024-11-05T00:06:42Z

tests/test_calibrateImage.py

@@ -595,6 +616,36 @@ def test_runQuantum_no_optional_outputs(self):
            # Restore the one we removed for the next test.
            connections[optional] = temp

+    def test_runQuantum_no_calibrate_pixels(self):


Should note in the test docstring that this is testing that the applied_photo_calib output has been removed.

TallJimbo · 2024-11-05T15:08:02Z

Should setting this new config to False also require changing the exposure output name to something other than initial_pvi, so that we can keep track of which ones were calibrated and which not? That's my main worry here, since right now the only way to know is to check whether the attached PhotoCalib is ==1.

I'm a little worried about this, but I think putting constraints on the dataset type name might be the kind of fix that's worse than the problem, especially with the names still somewhat in flux.

To get at the root issue, what do you think about setting BUNIT in the FITS header? That's not as nice as some afw object that maps to and from the FITS header, but it's a very established external standard, and arguably all we really need for external users.

parejkoj · 2024-11-05T21:14:07Z

python/lsst/pipe/tasks/calibrateImage.py

@@ -949,6 +949,7 @@ def _apply_photometry(self, exposure, background):
                                       photo_calib.getCalibrationErr(),
                                       bbox=exposure.getBBox())
        exposure.setPhotoCalib(identity)
+        exposure.metadata["BUNIT"] = "nJy"


Although I'd much rather have this done within PhotoCalib itself, I guess it's not the worst thing to do it here, even better if we also do it in reprocessVisitImage, and the diffim and coadd code.

parejkoj · 2024-11-05T21:16:29Z

You don't have to do it on this ticket, but please file a ticket about setting BUNIT everywhere we're writing an nJy image, and/or getting PhotoCalib to do it automatically if possible.

TallJimbo · 2024-11-05T22:13:27Z

As you might have seen, I've already added BUNIT to CalibrateImageTask here, and I've created DM-47408 for the rest.

This resolves a TODO comment wondering whether computing summary stats after applying the photometric calibration would be a problem, because it is - at least for DRP, where FGCM depends on some quantities being in instrumental units. I'm guessing AP doesn't care, but this needs to be verified.

TallJimbo force-pushed the tickets/DM-46966 branch 2 times, most recently from a337ece to 8b8725f Compare October 20, 2024 12:26

erykoff reviewed Oct 21, 2024

View reviewed changes

TallJimbo force-pushed the tickets/DM-46966 branch from 8b8725f to 112fb5d Compare October 21, 2024 16:27

TallJimbo force-pushed the tickets/DM-46966 branch from 112fb5d to 19ee100 Compare November 1, 2024 18:36

parejkoj reviewed Nov 5, 2024

View reviewed changes

TallJimbo force-pushed the tickets/DM-46966 branch from 19ee100 to b51942e Compare November 5, 2024 15:03

parejkoj reviewed Nov 5, 2024

View reviewed changes

parejkoj approved these changes Nov 5, 2024

View reviewed changes

TallJimbo added 6 commits November 5, 2024 17:14

Add missing background parameter docs on _fit_photometry.

73cae20

Split CalibrateImage photometry fit from photometry apply.

3051a2d

Make calibrating pixels optional in CalibrateImageTask.

e818b39

Address style and doc issues from code review.

8bb3162

Set FITS BUNIT='nJy' in calibrateImage when calibrating pixels.

36afc7d

TallJimbo force-pushed the tickets/DM-46966 branch from b2c0eac to 36afc7d Compare November 5, 2024 22:14

TallJimbo merged commit b51dc15 into main Nov 6, 2024
2 checks passed

TallJimbo deleted the tickets/DM-46966 branch November 6, 2024 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-46966: Make calibrating pixel values optional in CalibrateImageTask #996

DM-46966: Make calibrating pixel values optional in CalibrateImageTask #996

TallJimbo commented Oct 19, 2024

erykoff Oct 21, 2024

erykoff Oct 21, 2024

parejkoj left a comment

parejkoj Nov 4, 2024

parejkoj Nov 4, 2024

TallJimbo Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

TallJimbo Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj Nov 5, 2024

TallJimbo commented Nov 5, 2024

parejkoj Nov 5, 2024

parejkoj commented Nov 5, 2024

TallJimbo commented Nov 5, 2024 •

edited by jira bot

Loading


		This is `None` if ``config.do_calibrate_pixels`` is `False`.

		background : `lsst.afw.math.BackgroundList`
		Background model to convert to nanojansky units in place.

	photo_calib = result.exposure.getPhotoCalib()
	photo_calib = result.exposure.photoCalib

	self.assertEqual(result.exposure.getPhotoCalib().getCalibrationMean(), 1.0)
	self.assertEqual(result.exposure.photoCalib.getCalibrationMean(), 1.0)

		"""Test that run() returns reasonable values to be butler put when
		do_calibrate_pixels=False.

DM-46966: Make calibrating pixel values optional in CalibrateImageTask #996

DM-46966: Make calibrating pixel values optional in CalibrateImageTask #996

Conversation

TallJimbo commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj 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

TallJimbo commented Nov 5, 2024

Choose a reason for hiding this comment

parejkoj commented Nov 5, 2024

TallJimbo commented Nov 5, 2024 • edited by jira bot Loading

TallJimbo commented Nov 5, 2024 •

edited by jira bot

Loading