DM-45042: ComputeExposureSummaryStats needs to be able to turn off "update" code #977

laurenam · 2024-09-10T00:32:37Z

No description provided.

parejkoj

I think you need to strip this down to only have "doX" configs for the slowest components. The rest of the basic values we should always try to calculate. There's no point in adding "doX" settings for things that we'll always want to compute.

parejkoj · 2024-09-20T06:54:29Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+    doUpdatePsfStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Do update the psf statistics. Note that the PSF and apCorr model fidelity "


For all of the "doX", don't repeat the "do" in the docstring. For example:

Suggested change

doc="Do update the psf statistics. Note that the PSF and apCorr model fidelity "

doc="Update the psf statistics? Note that the PSF and apCorr model fidelity "

parejkoj · 2024-09-20T06:56:05Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+        "delta metrics are tied up in the same function as the more basic PSF metrics (e.g. "
+        "psfSigma and size/shape residuals). If the basic ones are still desired, but the "
+        "computation-intesive delta metrics are not, leave this as true and set "
+        "config.psfGridSampling to None. Set to False if speed is of the essence.",


Are the psfSigma metrics slow at all? I think we only need to turn off the calculations that really matter. psfSigma seems like such a basic value that it should always be calculated if possible. Otherwise, what else is even computed here?

Agreed. I removed that as an option and added a note in the comments which parameters are always (i.e. non-optionally) run when this task is called. I also changed the grid-based ones to doUpdateX-style config options (which I do think is more obvious than setting the sampling sizes to None. The ap_pipe commit was updated to reflect this.)

parejkoj · 2024-09-20T07:09:54Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+            self.log.info("Note: not computing grid-based maxDistToNearestPsf model fidelity. Setting "
+                          "it to NaN.")


No need to say "setting it to NaN", that's the default value of all non-computed things.

parejkoj · 2024-09-20T07:13:35Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+            self.log.info("Note: not computing grid-based PSF & ApCorr model fidelity metrics. Setting "
+                          "psfTraceRadiusDelta, psfApFluxDelta, & psfApCorrSigmaScaledDelta to NaN.")
+        else:
+            if image_mask is not None:


Why is image_mask optional? I can't think of any case where we'd not have access to the mask, and certainly the three places that we call this method, we pass a mask.

The mask being an optional input is pre-existing from over 2 years ago (e654c55), so I'm not inclined to question or change it (making it non-optional would change the API, and since it can be None, we need to check for it). I added a log info to note if these metrics were not asked to be turned off but no mask was provided. If you're really bothered by it as is, please feel free to pursue a deprecation!

Please ask about that on slack and file a ticket if there's no known reason: as I said, all three places in the stack that currently call this pass mask, and it doesn't make sense for it to be optional to me.

I'm gonna push back on this again. It's pre-existing and unrelated to this ticket. I am not bothered by it, so would not feel inclined to weigh in on, let alone initiate, a discussion about it. If you feel strongly about it, proceed as you will!

parejkoj · 2024-09-20T07:14:24Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

-            )
-            summary.maxDistToNearestPsf = float(maxDistToNearestPsf)
+        if self.config.psfSampling is None:
+            self.log.info("Note: not computing grid-based maxDistToNearestPsf model fidelity. Setting "


I'm not sure I understand what "model fidelity" means here in context?

See above. I will make sure to fix both if we decide to change the descriptive.

parejkoj · 2024-09-20T07:14:54Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

-                summary.psfApCorrSigmaScaledDelta = float(psfApCorrSigmaScaledDelta)
+        if self.config.psfGridSampling is None:
+            self.log.info("Note: not computing grid-based PSF & ApCorr model fidelity metrics. Setting "
+                          "psfTraceRadiusDelta, psfApFluxDelta, & psfApCorrSigmaScaledDelta to NaN.")


No need to say you're setting them to NaN, that's the default for uncomputed values.

parejkoj · 2024-09-20T07:15:07Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

-                    )
-                summary.psfApCorrSigmaScaledDelta = float(psfApCorrSigmaScaledDelta)
+        if self.config.psfGridSampling is None:
+            self.log.info("Note: not computing grid-based PSF & ApCorr model fidelity metrics. Setting "


Not sure what "model fidelity" means here?

Yeah...I struggled with the best descriptive to use here. I'm looking for a concise way to describe the robustness/reliability/(I landed on)fidelity of the PSF/apCorr model (largely in the context of potentially dangerous extrapolations that seem to be allowed in some of the modeling algorithms and reveals itself as a larger-than-realistic variation of fit parameters across a single detector). Do you have a suggestion/preference?

Why not be explicit about it? "PSF & ApCorr model minimum/maximum range metrics"? From the docstrings on those functions, that seems like what they are, no?

parejkoj · 2024-09-27T22:30:37Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+    doUpdateMaxDistToNearestPsfStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the grid-based maximun distance to the nearest PSF star fidelity statistic "


Suggested change

doc="Update the grid-based maximun distance to the nearest PSF star fidelity statistic "

doc="Update the grid-based maximum distance to the nearest PSF star fidelity statistic "

parejkoj · 2024-09-27T22:31:24Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

+    doUpdateWcsStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the wcs statistics? Set to False if speed is of the essence.",
+    )
+    doUpdatePhotoCalibStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the photoCalib statistics? Set to False if speed is of the essence.",
+    )
+    doUpdateBackgroundStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the background statistics? Set to False if speed is of the essence.",
+    )
+    doUpdateMaskedImageStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the masked image (i.e. skyNoise & meanVar) statistics? Set to False "
+        "if speed is of the essence.",
+    )
+    doUpdateEffectiveTimeStats = pexConfig.Field(
+        dtype=bool,
+        default=True,
+        doc="Update the effective time statistics? Set to False if speed is of the essence.",
+    )


I think you should remove all of these except the grid-based do flags: the statistics computed on a grid are the ones that take a long time, but the rest should all be fast enough that we'd never want to disable them, I'd think.

Huh, just want to double check that you really don't want control over any but the grid-based ones. When we discussed this (way back when!), I was left with the strong feeling that you wanted control over almost all of them for AP (e.g. in the ticket description you mention update_wcs_stats and update_masked_image_deltas taking >1s per image as being a problem). Does even if AP decides to keep many of them at present, does it hurt to have the option already available in case opinions on that change (for AP or other caller of this task)?

If you do want these removed, just for the record, can you specify on the ticket that you no longer consider any but the grid-based computations an issue?

I've waffled back and forth on this a bit. The reason to not have doX configs for things that there would be no real reason to turn off is that it makes the code and config listings more complicated for no real benefit. I'll go add a comment on the ticket about this.

I've been doing some profiling but wanted to run on some OR4 data (which is more realistic than DC2) to get a better sense of the slow points.

Ok, thanks. I’ll hang tight!

parejkoj · 2024-09-27T22:35:46Z

python/lsst/pipe/tasks/computeExposureSummaryStats.py

-                    )
-                summary.psfApCorrSigmaScaledDelta = float(psfApCorrSigmaScaledDelta)
+        if self.config.psfGridSampling is None:
+            self.log.info("Note: not computing grid-based PSF & ApCorr model fidelity metrics. Setting "


Why not be explicit about it? "PSF & ApCorr model minimum/maximum range metrics"? From the docstrings on those functions, that seems like what they are, no?

parejkoj · 2024-09-27T22:37:55Z

tests/test_computeExposureSummaryStats.py

+        self.assertTrue(summary.ra, nan)
+        self.assertTrue(summary.dec, nan)


assertTrue? I don't think this is doing what you think it is:

https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertTrue

Did you mean self.assertTrue(np.isnan(summary.ra)), and similarly for all the tests below? Note that you cannot test equality for NaN, because it's not equal to itself.

Thanks for catching that!

Computation of these statistics can be time consuming, so this adds an option to turn any of them off for scenarios where they are not useful (which they would be for, e.g., downstream processing decision making) and/or processing time minimization is of the essence.

laurenam requested a review from parejkoj September 10, 2024 00:32

laurenam force-pushed the tickets/DM-45042 branch from 0fe9c25 to b50b68d Compare September 11, 2024 01:38

laurenam force-pushed the tickets/DM-45042 branch from b50b68d to 5a66a3b Compare September 18, 2024 23:40

parejkoj requested changes Sep 20, 2024

View reviewed changes

laurenam force-pushed the tickets/DM-45042 branch 2 times, most recently from cf48856 to 53829ff Compare September 23, 2024 00:41

parejkoj reviewed Sep 27, 2024

View reviewed changes

laurenam force-pushed the tickets/DM-45042 branch from 53829ff to 10b42ef Compare September 29, 2024 22:00

laurenam force-pushed the tickets/DM-45042 branch from 10b42ef to 8f70e30 Compare October 9, 2024 19:32

laurenam added 3 commits November 12, 2024 15:03

Add unittest for disabling summary stats updates

70df526

Remove no longer appropriate calibreteImage overrides

e978344

laurenam force-pushed the tickets/DM-45042 branch from 8f70e30 to e978344 Compare November 12, 2024 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-45042: ComputeExposureSummaryStats needs to be able to turn off "update" code #977

DM-45042: ComputeExposureSummaryStats needs to be able to turn off "update" code #977

laurenam commented Sep 10, 2024

parejkoj left a comment

parejkoj Sep 20, 2024

parejkoj Sep 20, 2024

laurenam Sep 22, 2024

parejkoj Sep 20, 2024

parejkoj Sep 20, 2024

laurenam Sep 21, 2024

parejkoj Sep 27, 2024

laurenam Sep 29, 2024

parejkoj Sep 20, 2024

laurenam Sep 21, 2024

parejkoj Sep 20, 2024

parejkoj Sep 20, 2024

laurenam Sep 21, 2024

parejkoj Sep 27, 2024

parejkoj Sep 27, 2024

parejkoj Sep 27, 2024 •

edited

Loading

laurenam Sep 29, 2024

parejkoj Oct 11, 2024

laurenam Oct 11, 2024

parejkoj Sep 27, 2024

parejkoj Sep 27, 2024

laurenam Sep 29, 2024

	doc="Do update the psf statistics. Note that the PSF and apCorr model fidelity "
	doc="Update the psf statistics? Note that the PSF and apCorr model fidelity "

		self.log.info("Note: not computing grid-based maxDistToNearestPsf model fidelity. Setting "
		"it to NaN.")

	doc="Update the grid-based maximun distance to the nearest PSF star fidelity statistic "
	doc="Update the grid-based maximum distance to the nearest PSF star fidelity statistic "

		self.assertTrue(summary.ra, nan)
		self.assertTrue(summary.dec, nan)

DM-45042: ComputeExposureSummaryStats needs to be able to turn off "update" code #977

Are you sure you want to change the base?

DM-45042: ComputeExposureSummaryStats needs to be able to turn off "update" code #977

Conversation

laurenam commented Sep 10, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Sep 27, 2024 • 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

parejkoj Sep 27, 2024 •

edited

Loading