DM-38546: Implement new CalibrateImageTask #802

parejkoj · 2023-06-23T22:26:03Z

No description provided.

TallJimbo · 2023-07-10T16:32:37Z

python/lsst/pipe/tasks/photoCal.py

@@ -417,6 +417,9 @@ def run(self, exposure, sourceCat, expId=0):
        flux0 = 10**(0.4*r.zp)  # Flux of mag=0 star
        flux0err = 0.4*math.log(10)*flux0*r.sigma  # Error in flux0
        photoCalib = makePhotoCalibFromCalibZeroPoint(flux0, flux0err)
+        self.log.info("Photometric calibration factor (nJy/ADU): %f +/- %f",
+                      photoCalib.getCalibrationMean(),
+                      photoCalib.getCalibrationErr())


Could you roll this into the previous log message or set it to DEBUG? I don't think we usually want this task to be very verbose.

or use self.log.verbose if this is not really a debug message but also not something you want people to see all the time. Pipeline batch execution runs at VERBOSE level rather than INFO.

PhotoCalTask is very much not chatty:

Are color terms being applied (and if so, from where)?

Report result as a magnitude zero point.

Report result as calibration factor (this addition).

We still don't have clear guidance on what to output at what level, but two lines summarizing what the result is seems appropriate here.

I don't have a very strong opinion. I just think people tend to look at task-level log messages in order to:

help diagnose a problem

get a quick sense of status or timing (there are better ways for both, but probably not easier ways)

and I don't see this one as helping with either.

TallJimbo · 2023-07-10T16:33:54Z

python/lsst/pipe/tasks/calibrateImage.py

+
+    astrometry_ref_cat = connectionTypes.PrerequisiteInput(
+        doc="Reference catalog to use for astrometric calibration.",
+        name="gaia_dr2_20200414",


Keep an eye on Clare's Gaia DR3 ticket in case it lands before this does.

I don't think we'll have dr3 distributed across our various test datasets nor fully vetted soon enough. It's an easy change here once that happens.

TallJimbo · 2023-07-10T16:35:11Z

python/lsst/pipe/tasks/calibrateImage.py

+        storageClass="ExposureF",
+        dimensions=("instrument", "visit", "detector"),
+    )
+    # TODO: persist a parquet version of this!


Not for this ticket, I assume?

I'm not sure: we'll need the SourceCatalog versions to store the footprints, but ~~Eli's~~ your vision is to only store the footprints, none of the other results in that SourceCatalog. I'm not sure that's actually practical here.

In order to get this merged, I guess I'll put this on the "do it later" list, but it should probably be near the top of that list?

Filed as DM-40061.

TallJimbo · 2023-07-10T16:35:59Z

python/lsst/pipe/tasks/calibrateImage.py

+    stars = connectionTypes.Output(
+        doc="Catalog of unresolved sources detected on the calibrated exposure; "
+            "includes source footprints.",
+        name="initial_stars",  # TODO: what to name this?


This TODO should definitely be done on this ticket, and that includes thinking about what to name it now if we want the long-term name to be the parquet variant, because adding new dataset types is a gazillion times easier than changing the meaning of old ones.

initial_stars_footprints_detector, in keeping with the "this is where you get the source footprints" thing, and also the "append _detector so full-visit things don't need a suffix"? And then the parquet version would be initial_stars_detector?

If instead we called this initial_stars_detector, what would we call the parquet version?

I'm pretty happy with initial_stars_footprints_detector for FITS and initial_stars_detector for parquet.

Do we need the _detector on the AFW tables? They're never aggregated, as far as I'm aware. If we want _detector on all detector-level catalogs, no matter their format, that's fine.

python/lsst/pipe/tasks/calibrateImage.py

tests/test_calibrateImage.py

TallJimbo · 2023-07-10T18:02:13Z

tests/test_calibrateImage.py

+        # We don't have many sources, so have to fit simpler models.
+        self.config.psf_detection.background.approxOrderX = 1
+        self.config.star_detection.background.approxOrderX = 1
+        # TODO: While debugging DM-32701, we're using PCA instead of psfex.


Is this TODO a relic now?

Unfortunately, no. DM-32701 fixed the "junk PSF produced" problem into a "raise error", but we still have the same problem of it thinking we don't have enough PSF sources (even when we should for the given fitting order). We don't have a followup ticket to explore that further, so I'm not sure what to say here.

If this is just about the test, not any real data, let's just remove the TODO.

PSFEx seeming inconsistent about how many stars it needs is something I'd consider a low-priority problem (to the point where I'm not sure a ticket or TODO is merited), because it's easy to imagine that happening because our thinking of its algorithm is an oversimplification and it genuinely has more degrees of freedom.

If the problem is that PSFEx just isn't robust enough on some important class of data, that's different.

I think the problem is that we don't know which of those cases it is. This is basically the problem described on DM-40001. In this case, we can just use PCA since the tests don't really care, but I think we probably should investigate psfex's failure modes more fully here.

TallJimbo · 2023-07-10T18:06:20Z

tests/test_calibrateImage.py

+        idx, _, _ = fitted.match_to_catalog_sky(truth)
+        # TODO: because the input variance image does not include contributions
+        # from the sources, the measured instFluxes do not reflect the greater
+        # uncertainty for faint sources; we can't use fluxErr as a bound on


Do you mean "greater uncertainty for bright sources"?

I've reworded it.

It's worth logging this in addition to the zero point logged above, as this is the value that is saved in the PhotoCalib object.

gen3 pipetasks call setRefObjLoader; users in notebooks and tests may still want to use the __init__ interface though.

This is necessary for writing the denormalized match catalog.

parejkoj · 2023-07-25T00:40:19Z

I've also added in an "install simple PSF after initial PSF determination", per conversation on slack last week.

TallJimbo · 2023-07-26T17:00:38Z

python/lsst/pipe/tasks/photoCal.py

@@ -417,6 +417,9 @@ def run(self, exposure, sourceCat, expId=0):
        flux0 = 10**(0.4*r.zp)  # Flux of mag=0 star
        flux0err = 0.4*math.log(10)*flux0*r.sigma  # Error in flux0
        photoCalib = makePhotoCalibFromCalibZeroPoint(flux0, flux0err)
+        self.log.info("Photometric calibration factor (nJy/ADU): %f +/- %f",
+                      photoCalib.getCalibrationMean(),
+                      photoCalib.getCalibrationErr())


I don't have a very strong opinion. I just think people tend to look at task-level log messages in order to:

help diagnose a problem

get a quick sense of status or timing (there are better ways for both, but probably not easier ways)

and I don't see this one as helping with either.

TallJimbo · 2023-07-26T17:09:15Z

tests/test_calibrateImage.py

+        # We don't have many sources, so have to fit simpler models.
+        self.config.psf_detection.background.approxOrderX = 1
+        self.config.star_detection.background.approxOrderX = 1
+        # TODO: While debugging DM-32701, we're using PCA instead of psfex.


If this is just about the test, not any real data, let's just remove the TODO.

PSFEx seeming inconsistent about how many stars it needs is something I'd consider a low-priority problem (to the point where I'm not sure a ticket or TODO is merited), because it's easy to imagine that happening because our thinking of its algorithm is an oversimplification and it genuinely has more degrees of freedom.

If the problem is that PSFEx just isn't robust enough on some important class of data, that's different.

TallJimbo · 2023-07-26T17:15:31Z

python/lsst/pipe/tasks/calibrateImage.py

+    stars = connectionTypes.Output(
+        doc="Catalog of unresolved sources detected on the calibrated exposure; "
+            "includes source footprints.",
+        name="initial_stars",  # TODO: what to name this?


I'm pretty happy with initial_stars_footprints_detector for FITS and initial_stars_detector for parquet.

This task will replace CharacterizeImageTask and CalibrateTask.

parejkoj force-pushed the tickets/DM-38546 branch 4 times, most recently from 46d8a97 to 4620202 Compare June 30, 2023 00:05

TallJimbo approved these changes Jul 10, 2023

View reviewed changes

parejkoj force-pushed the tickets/DM-38546 branch from 4620202 to 43c1c4e Compare July 17, 2023 20:02

parejkoj added 3 commits July 24, 2023 17:14

Log photoCalib result in PhotoCalTask

f248aa7

It's worth logging this in addition to the zero point logged above, as this is the value that is saved in the PhotoCalib object.

Make refObjLoader optional

4253110

gen3 pipetasks call setRefObjLoader; users in notebooks and tests may still want to use the __init__ interface though.

Return matchMeta in PhotoCalTask

8cc778a

This is necessary for writing the denormalized match catalog.

parejkoj force-pushed the tickets/DM-38546 branch from 2fd8931 to da196b1 Compare July 25, 2023 00:35

TallJimbo approved these changes Jul 26, 2023

View reviewed changes

Add CalibrateImageTask, with tests

db7a6e0

This task will replace CharacterizeImageTask and CalibrateTask.

parejkoj force-pushed the tickets/DM-38546 branch from ff3c5f9 to db7a6e0 Compare July 28, 2023 01:23

parejkoj merged commit 2ebef2b into main Jul 31, 2023
2 checks passed

parejkoj deleted the tickets/DM-38546 branch July 31, 2023 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-38546: Implement new CalibrateImageTask #802

DM-38546: Implement new CalibrateImageTask #802

parejkoj commented Jun 23, 2023

TallJimbo Jul 10, 2023

timj Jul 10, 2023

parejkoj Jul 15, 2023

TallJimbo Jul 26, 2023

TallJimbo Jul 10, 2023

parejkoj Jul 15, 2023

TallJimbo Jul 10, 2023

parejkoj Jul 15, 2023 •

edited

Loading

parejkoj Jul 24, 2023

TallJimbo Jul 10, 2023

parejkoj Jul 15, 2023

TallJimbo Jul 26, 2023

parejkoj Jul 26, 2023

TallJimbo Jul 10, 2023

parejkoj Jul 25, 2023

TallJimbo Jul 26, 2023

parejkoj Jul 28, 2023

TallJimbo Jul 10, 2023

parejkoj Jul 25, 2023

parejkoj commented Jul 25, 2023

TallJimbo Jul 26, 2023

TallJimbo Jul 26, 2023

TallJimbo Jul 26, 2023

DM-38546: Implement new CalibrateImageTask #802

DM-38546: Implement new CalibrateImageTask #802

Conversation

parejkoj commented Jun 23, 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

parejkoj Jul 15, 2023 • edited Loading

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

parejkoj commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Jul 15, 2023 •

edited

Loading