From a89c7073e80834b230997b99ded58f5802128c38 Mon Sep 17 00:00:00 2001 From: "J. Bryce Kalmbach" Date: Thu, 7 Nov 2024 13:44:35 -0800 Subject: [PATCH 1/4] Add group pairer version of AggregateDonutTablesTask in place of default. --- pipelines/production/comCamRapidAnalysisPipeline.yaml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pipelines/production/comCamRapidAnalysisPipeline.yaml b/pipelines/production/comCamRapidAnalysisPipeline.yaml index d0779a82..ce99f05d 100644 --- a/pipelines/production/comCamRapidAnalysisPipeline.yaml +++ b/pipelines/production/comCamRapidAnalysisPipeline.yaml @@ -36,8 +36,12 @@ tasks: crosstalk.doQuadraticCrosstalkCorrection: False aggregateZernikeTablesTask: class: lsst.donut.viz.AggregateZernikeTablesTask - aggregateDonutTablesTask: - class: lsst.donut.viz.AggregateDonutTablesTask + aggregateDonutTablesGroupTask: + class: lsst.donut.viz.AggregateDonutTablesTask + config: + python: | + from lsst.ts.wep.task.pairTask import GroupPairer + config.pairer.retarget(GroupPairer) aggregateAOSVisitTableTask: class: lsst.donut.viz.AggregateAOSVisitTableTask plotAOSTask: From c791ec609004a5f1c108f9c5067f550fdacbba57 Mon Sep 17 00:00:00 2001 From: "J. Bryce Kalmbach" Date: Thu, 7 Nov 2024 14:21:29 -0800 Subject: [PATCH 2/4] Fix pipeline steps. --- pipelines/production/comCamRapidAnalysisPipeline.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipelines/production/comCamRapidAnalysisPipeline.yaml b/pipelines/production/comCamRapidAnalysisPipeline.yaml index ce99f05d..1075b0b7 100644 --- a/pipelines/production/comCamRapidAnalysisPipeline.yaml +++ b/pipelines/production/comCamRapidAnalysisPipeline.yaml @@ -70,7 +70,7 @@ subsets: step2a: subset: - aggregateZernikeTablesTask - - aggregateDonutTablesTask + - aggregateDonutTablesGroupTask - aggregateAOSVisitTableTask - plotAOSTask - aggregateDonutStampsTask From c5f0ec632991627d92c321b19911ca1645e4b2e8 Mon Sep 17 00:00:00 2001 From: Bryce Kalmbach Date: Thu, 7 Nov 2024 17:15:14 -0800 Subject: [PATCH 3/4] Update versionHistory.rst --- doc/versionHistory.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/versionHistory.rst b/doc/versionHistory.rst index c7d747dc..a9b7f5a5 100644 --- a/doc/versionHistory.rst +++ b/doc/versionHistory.rst @@ -8,6 +8,14 @@ Version History .. _lsst.ts.wep-12.6.1: +------------- +12.6.2 +------------- + +* Update RA production pipeline to use group dimension in aggregate donut tables step. + +.. _lsst.ts.wep-12.6.1: + ------------- 12.6.1 ------------- From a47198e16f95555030cec0599754df0ab527f37d Mon Sep 17 00:00:00 2001 From: John Franklin Crenshaw Date: Wed, 6 Nov 2024 17:51:23 -0800 Subject: [PATCH 4/4] requireConverge in TIE; moved binning config to estimateZernikes derived classes; fixed Nones in task history. --- doc/versionHistory.rst | 9 ++++++ .../comCamRapidAnalysisPipeline.yaml | 4 ++- python/lsst/ts/wep/estimation/tie.py | 29 +++++++++++++++++++ .../lsst/ts/wep/task/estimateZernikesBase.py | 6 ---- .../ts/wep/task/estimateZernikesDanishTask.py | 6 ++++ .../ts/wep/task/estimateZernikesTieTask.py | 12 ++++++++ python/lsst/ts/wep/utils/taskUtils.py | 9 ++++-- tests/task/test_calcZernikesTieTaskCwfs.py | 23 +++++++++++++++ 8 files changed, 88 insertions(+), 10 deletions(-) diff --git a/doc/versionHistory.rst b/doc/versionHistory.rst index a9b7f5a5..91f0c718 100644 --- a/doc/versionHistory.rst +++ b/doc/versionHistory.rst @@ -6,6 +6,15 @@ Version History ################## +.. _lsst.ts.wep-12.7.0: + +------------- +12.7.0 +------------- + +* Added requireConverge to TIE and defaulted to True in task +* Fixed bug with None types in EstimateZernikeTask metadata histories + .. _lsst.ts.wep-12.6.1: ------------- diff --git a/pipelines/production/comCamRapidAnalysisPipeline.yaml b/pipelines/production/comCamRapidAnalysisPipeline.yaml index 1075b0b7..375f2900 100644 --- a/pipelines/production/comCamRapidAnalysisPipeline.yaml +++ b/pipelines/production/comCamRapidAnalysisPipeline.yaml @@ -17,7 +17,9 @@ tasks: calcZernikesTask: class: lsst.ts.wep.task.calcZernikesTask.CalcZernikesTask config: - estimateZernikes.maxNollIndex: 28 + estimateZernikes.maxNollIndex: 22 + estimateZernikes.convergeTol: 10.0e-9 + estimateZernikes.requireConverge: True estimateZernikes.saveHistory: False estimateZernikes.maskKwargs: { "doMaskBlends": False } donutStampSelector.maxSelect: 5 diff --git a/python/lsst/ts/wep/estimation/tie.py b/python/lsst/ts/wep/estimation/tie.py index 034968f9..80058ea5 100644 --- a/python/lsst/ts/wep/estimation/tie.py +++ b/python/lsst/ts/wep/estimation/tie.py @@ -95,6 +95,11 @@ class TieAlgorithm(WfAlgorithm): binning : int, optional Binning factor to apply to the donut stamps before estimating Zernike coefficients. The default value of 1 means no binning. + (the default is 1) + requireConverge : bool, optional + Whether to require that the TIE converges. If True, and the TIE + did not converge, the TIE returns NaNs. + (the default is False) """ def __init__( @@ -109,6 +114,7 @@ def __init__( maskKwargs: Optional[dict] = None, modelPupilKernelSize: float = 2, binning: int = 1, + requireConverge: bool = False, ) -> None: self.opticalModel = opticalModel self.maxIter = maxIter @@ -120,6 +126,7 @@ def __init__( self.maskKwargs = maskKwargs self.modelPupilKernelSize = modelPupilKernelSize self.binning = binning + self.requireConverge = requireConverge @property def requiresPairs(self) -> bool: @@ -429,6 +436,24 @@ def binning(self, value: int) -> None: raise ValueError("binning must be greater than or equal to 1.") self._binning = value + @property + def requireConverge(self) -> int: + """Whether to require that the TIE converges.""" + return self._requireConverge + + @requireConverge.setter + def requireConverge(self, value: int) -> None: + """Whether to require that the TIE converges. + + Parameters + ---------- + value : bool + Whether to require that the TIE converges. + """ + if not isinstance(value, bool): + raise TypeError("requireConverge must be a bool.") + self._requireConverge = value + @property def history(self) -> dict: """The algorithm history. @@ -867,4 +892,8 @@ def _estimateZk( if caustic or converged: break + # If we never converged, return NaNs? + if self.requireConverge and not converged: + zkSum *= np.nan + return zkSum diff --git a/python/lsst/ts/wep/task/estimateZernikesBase.py b/python/lsst/ts/wep/task/estimateZernikesBase.py index 34893b01..b52d69ac 100644 --- a/python/lsst/ts/wep/task/estimateZernikesBase.py +++ b/python/lsst/ts/wep/task/estimateZernikesBase.py @@ -69,12 +69,6 @@ class EstimateZernikesBaseConfig(pexConfig.Config): + "to the Noll index. In this case, indices 0-3 are always set to zero, " + "because they are not estimated by our pipeline.", ) - binning = pexConfig.Field( - dtype=int, - default=1, - doc="Binning factor to apply to the donut stamps before estimating " - + "Zernike coefficients. A value of 1 means no binning.", - ) saveHistory = pexConfig.Field( dtype=bool, default=False, diff --git a/python/lsst/ts/wep/task/estimateZernikesDanishTask.py b/python/lsst/ts/wep/task/estimateZernikesDanishTask.py index 5094a9a0..e68ff073 100644 --- a/python/lsst/ts/wep/task/estimateZernikesDanishTask.py +++ b/python/lsst/ts/wep/task/estimateZernikesDanishTask.py @@ -38,6 +38,12 @@ class EstimateZernikesDanishConfig(EstimateZernikesBaseConfig): doc="A dictionary containing any of the keyword arguments for " + "scipy.optimize.least_squares, except `fun`, `x0`, `jac`, or `args`.", ) + binning = pexConfig.Field( + dtype=int, + default=1, + doc="Binning factor to apply to the donut stamps before estimating " + + "Zernike coefficients. A value of 1 means no binning.", + ) class EstimateZernikesDanishTask(EstimateZernikesBaseTask): diff --git a/python/lsst/ts/wep/task/estimateZernikesTieTask.py b/python/lsst/ts/wep/task/estimateZernikesTieTask.py index f7d73e16..1069f744 100644 --- a/python/lsst/ts/wep/task/estimateZernikesTieTask.py +++ b/python/lsst/ts/wep/task/estimateZernikesTieTask.py @@ -104,6 +104,18 @@ class EstimateZernikesTieConfig(EstimateZernikesBaseConfig): + "when estimating Zernikes with a single donut. " + "(the default is 2)", ) + binning = pexConfig.Field( + dtype=int, + default=1, + doc="Binning factor to apply to the donut stamps before estimating " + + "Zernike coefficients. A value of 1 means no binning.", + ) + requireConverge = pexConfig.Field( + dtype=bool, + default=False, + doc="Whether to require that the TIE converges. " + + "If True, and the TIE did not converge, the TIE returns NaNs. ", + ) class EstimateZernikesTieTask(EstimateZernikesBaseTask): diff --git a/python/lsst/ts/wep/utils/taskUtils.py b/python/lsst/ts/wep/utils/taskUtils.py index c7c74bd2..32bacccf 100644 --- a/python/lsst/ts/wep/utils/taskUtils.py +++ b/python/lsst/ts/wep/utils/taskUtils.py @@ -446,7 +446,8 @@ def convertHistoryToMetadata(history: dict) -> pipeBase.TaskMetadata: """ if isinstance(history, dict): history = { - str(key): convertHistoryToMetadata(val) for key, val in history.items() + str(key): ("None" if val is None else convertHistoryToMetadata(val)) + for key, val in history.items() } history = pipeBase.TaskMetadata.from_dict(history) elif isinstance(history, np.ndarray): @@ -493,9 +494,11 @@ def convertMetadataToHistory(metadata: pipeBase.TaskMetadata) -> dict: for key, val in metadata.items() } - # Convert numeric strings back to floats and ints + # Convert "None" strings back to None and numeric strings to floats/ints elif isinstance(metadata, str): - if "." in metadata: + if metadata == "None": + metadata = None + elif "." in metadata: try: metadata = float(metadata) except: # noqa: E722 diff --git a/tests/task/test_calcZernikesTieTaskCwfs.py b/tests/task/test_calcZernikesTieTaskCwfs.py index 3005f82b..19e8f351 100644 --- a/tests/task/test_calcZernikesTieTaskCwfs.py +++ b/tests/task/test_calcZernikesTieTaskCwfs.py @@ -112,6 +112,9 @@ def testValidateConfigs(self): self.assertEqual(type(self.task.combineZernikes), CombineZernikesMeanTask) + self.config.estimateZernikes.binning = 2 + self.assertEqual(self.task.estimateZernikes.wfAlgoConfig.binning, 2) + def testEstimateZernikes(self): zernCoeff = self.task.estimateZernikes.run( self.donutStampsExtra, self.donutStampsIntra @@ -281,3 +284,23 @@ def testUnevenPairs(self): # Now estimate Zernikes self.task.run(stampsExtra, stampsIntra) + + def testRequireConverge(self): + config = CalcZernikesTaskConfig() + config.estimateZernikes.requireConverge = True # Require to converge + config.estimateZernikes.convergeTol = 0 # But don't allow convergence + task = CalcZernikesTask(config=config, name="Test requireConverge") + + # Estimate zernikes + donutStampDir = os.path.join(self.testDataDir, "donutImg", "donutStamps") + donutStampsExtra = DonutStamps.readFits( + os.path.join(donutStampDir, "R04_SW0_donutStamps.fits") + ) + donutStampsIntra = DonutStamps.readFits( + os.path.join(donutStampDir, "R04_SW1_donutStamps.fits") + ) + output = task.estimateZernikes.run(donutStampsExtra, donutStampsIntra) + zernikes = output.zernikes + + # Everything should be NaN because we did not converge + self.assertTrue(np.isnan(zernikes).all())