From 1adb2339675ca95d3cf3d51c7c2f9b7eb9a7530d Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 9 May 2024 08:12:36 -0400 Subject: [PATCH 1/9] Add a check for separation and tolerance in tweakreg --- CHANGES.rst | 5 +++++ docs/jwst/tweakreg/README.rst | 6 ++++-- jwst/tweakreg/tweakreg_step.py | 4 ++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index c353bf0c74..8b18a65672 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -217,6 +217,11 @@ tweakreg - Change code default to use IRAF StarFinder instead of DAO StarFinder [#8487] +- Added a check for ``(abs_)separation`` and ``(abs_)tolerance`` parameters + that ``separation`` > ``sqrt(2) * tolerance`` that will now raise an exception + when this condition is not satisfied and source confusion is possible during + catalog matching. [#8476] + wfss_contam ----------- diff --git a/docs/jwst/tweakreg/README.rst b/docs/jwst/tweakreg/README.rst index 5e707dbbb6..f24aa60f2d 100644 --- a/docs/jwst/tweakreg/README.rst +++ b/docs/jwst/tweakreg/README.rst @@ -307,7 +307,8 @@ The ``tweakreg`` step has the following optional arguments: * ``use2dhist``: A boolean indicating whether to use 2D histogram to find initial offset. (Default=True) -* ``separation``: Minimum object separation in arcsec. (Default=1.0) +* ``separation``: Minimum object separation in arcsec. It **must be** at least + ``sqrt(2)`` times larger than ``tolerance``. (Default=1.0) * ``tolerance``: Matching tolerance for ``xyxymatch`` in arcsec. (Default=0.7) @@ -374,7 +375,8 @@ Parameters used for absolute astrometry to a reference catalog. Otherwise the initial guess for the offsets will be set to zero (Default=True) -* ``abs_separation``: Minimum object separation in arcsec. (Default=1.0) +* ``abs_separation``: Minimum object separation in arcsec. It **must be** at + least ``sqrt(2)`` times larger than ``abs_tolerance``. (Default=1.0) * ``abs_tolerance``: Matching tolerance for ``xyxymatch`` in arcsec. (Default=0.7) diff --git a/jwst/tweakreg/tweakreg_step.py b/jwst/tweakreg/tweakreg_step.py index 287128bcc9..7f1e50ccad 100644 --- a/jwst/tweakreg/tweakreg_step.py +++ b/jwst/tweakreg/tweakreg_step.py @@ -5,6 +5,7 @@ """ from os import path +import math from astropy import units as u from astropy.coordinates import SkyCoord @@ -23,6 +24,9 @@ from .tweakreg_catalog import make_tweakreg_catalog +_SQRT2 = math.sqrt(2.0) + + def _oxford_or_str_join(str_list): nelem = len(str_list) if not nelem: From fb116cbc814f5a7f971139d041496f7192c3ba05 Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 16 May 2024 14:17:20 -0400 Subject: [PATCH 2/9] Change to log message instead or raising exceptions --- jwst/tweakreg/tweakreg_step.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/jwst/tweakreg/tweakreg_step.py b/jwst/tweakreg/tweakreg_step.py index 7f1e50ccad..69adc54a31 100644 --- a/jwst/tweakreg/tweakreg_step.py +++ b/jwst/tweakreg/tweakreg_step.py @@ -161,6 +161,34 @@ def process(self, input): else: catdict[member.expname] = path.join(asn_dir, member.tweakreg_catalog) + if self.separation <= _SQRT2 * self.tolerance: + self.log.error( + "Parameter 'separation' must be larger than 'tolerance' by at " + "least a factor of sqrt(2) to avoid source confusion." + ) + for model in images: + model.meta.cal_step.tweakreg = "SKIPPED" + # Remove the attached catalogs + if hasattr(model, "catalog"): + del model.catalog + self.skip = True + self.log.warning("Skipping 'TweakRegStep' step.") + return images + + if self.abs_separation <= _SQRT2 * self.abs_tolerance: + self.log.error( + "Parameter 'abs_separation' must be larger than 'abs_tolerance' " + "by at least a factor of sqrt(2) to avoid source confusion." + ) + for model in images: + model.meta.cal_step.tweakreg = "SKIPPED" + # Remove the attached catalogs + if hasattr(model, "catalog"): + del model.catalog + self.skip = True + self.log.warning("Skipping 'TweakRegStep' step.") + return images + if self.abs_refcat is not None and self.abs_refcat.strip(): align_to_abs_refcat = True # Set expand_refcat to True to eliminate possibility of duplicate From 38d4d64920f5e34414a77138e527f2113f5569b5 Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 16 May 2024 15:43:24 -0400 Subject: [PATCH 3/9] remove redundant code --- jwst/tweakreg/tweakreg_step.py | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/jwst/tweakreg/tweakreg_step.py b/jwst/tweakreg/tweakreg_step.py index 69adc54a31..79abb41043 100644 --- a/jwst/tweakreg/tweakreg_step.py +++ b/jwst/tweakreg/tweakreg_step.py @@ -166,25 +166,15 @@ def process(self, input): "Parameter 'separation' must be larger than 'tolerance' by at " "least a factor of sqrt(2) to avoid source confusion." ) - for model in images: - model.meta.cal_step.tweakreg = "SKIPPED" - # Remove the attached catalogs - if hasattr(model, "catalog"): - del model.catalog self.skip = True self.log.warning("Skipping 'TweakRegStep' step.") return images if self.abs_separation <= _SQRT2 * self.abs_tolerance: self.log.error( - "Parameter 'abs_separation' must be larger than 'abs_tolerance' " - "by at least a factor of sqrt(2) to avoid source confusion." - ) - for model in images: - model.meta.cal_step.tweakreg = "SKIPPED" - # Remove the attached catalogs - if hasattr(model, "catalog"): - del model.catalog + "Parameter 'abs_separation' must be larger than 'abs_tolerance' " + "by at least a factor of sqrt(2) to avoid source confusion." + ) self.skip = True self.log.warning("Skipping 'TweakRegStep' step.") return images From fe4ec9f19b5b1cfb8c679bb8312b5ec16a1b0b83 Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 16 May 2024 15:52:20 -0400 Subject: [PATCH 4/9] move check too the top --- jwst/tweakreg/tweakreg_step.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/jwst/tweakreg/tweakreg_step.py b/jwst/tweakreg/tweakreg_step.py index 79abb41043..7f1e50ccad 100644 --- a/jwst/tweakreg/tweakreg_step.py +++ b/jwst/tweakreg/tweakreg_step.py @@ -161,24 +161,6 @@ def process(self, input): else: catdict[member.expname] = path.join(asn_dir, member.tweakreg_catalog) - if self.separation <= _SQRT2 * self.tolerance: - self.log.error( - "Parameter 'separation' must be larger than 'tolerance' by at " - "least a factor of sqrt(2) to avoid source confusion." - ) - self.skip = True - self.log.warning("Skipping 'TweakRegStep' step.") - return images - - if self.abs_separation <= _SQRT2 * self.abs_tolerance: - self.log.error( - "Parameter 'abs_separation' must be larger than 'abs_tolerance' " - "by at least a factor of sqrt(2) to avoid source confusion." - ) - self.skip = True - self.log.warning("Skipping 'TweakRegStep' step.") - return images - if self.abs_refcat is not None and self.abs_refcat.strip(): align_to_abs_refcat = True # Set expand_refcat to True to eliminate possibility of duplicate From ac468e7fe526e7431bd6826d1d51b345fe9776eb Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 23 May 2024 19:07:19 -0400 Subject: [PATCH 5/9] address reviewer comments --- CHANGES.rst | 2 +- jwst/tweakreg/tweakreg_step.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index 8b18a65672..dd677fea31 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -218,7 +218,7 @@ tweakreg DAO StarFinder [#8487] - Added a check for ``(abs_)separation`` and ``(abs_)tolerance`` parameters - that ``separation`` > ``sqrt(2) * tolerance`` that will now raise an exception + that ``separation`` > ``sqrt(2) * tolerance`` that will now raise an error when this condition is not satisfied and source confusion is possible during catalog matching. [#8476] diff --git a/jwst/tweakreg/tweakreg_step.py b/jwst/tweakreg/tweakreg_step.py index 7f1e50ccad..6b9fef82b7 100644 --- a/jwst/tweakreg/tweakreg_step.py +++ b/jwst/tweakreg/tweakreg_step.py @@ -128,6 +128,26 @@ class TweakRegStep(Step): def process(self, input): images = ModelContainer(input) + if self.separation <= _SQRT2 * self.tolerance: + self.log.error( + "Parameter 'separation' must be larger than 'tolerance' by at " + "least a factor of sqrt(2) to avoid source confusion." + ) + for model in images: + model.meta.cal_step.tweakreg = "SKIPPED" + self.log.warning("Skipping 'TweakRegStep' step.") + return input + + if self.abs_separation <= _SQRT2 * self.abs_tolerance: + self.log.error( + "Parameter 'abs_separation' must be larger than 'abs_tolerance' " + "by at least a factor of sqrt(2) to avoid source confusion." + ) + for model in images: + model.meta.cal_step.tweakreg = "SKIPPED" + self.log.warning("Skipping 'TweakRegStep' step.") + return input + if len(images) == 0: raise ValueError("Input must contain at least one image model.") From 9303790037abbaca85b0f351c0f9cdcb128d1f4f Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Thu, 23 May 2024 19:10:11 -0400 Subject: [PATCH 6/9] address reviewer comments --- CHANGES.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index dd677fea31..4465bfd5de 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -218,9 +218,9 @@ tweakreg DAO StarFinder [#8487] - Added a check for ``(abs_)separation`` and ``(abs_)tolerance`` parameters - that ``separation`` > ``sqrt(2) * tolerance`` that will now raise an error - when this condition is not satisfied and source confusion is possible during - catalog matching. [#8476] + that ``separation`` > ``sqrt(2) * tolerance`` that will now log an error + message and skip ``tweakreg`` step when this condition is not satisfied and + source confusion is possible during catalog matching. [#8476] wfss_contam ----------- From 857a3102847e3967a88e920312956fc6059475ae Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Sun, 26 May 2024 01:01:50 -0400 Subject: [PATCH 7/9] add unit tests --- jwst/tweakreg/tests/test_tweakreg.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/jwst/tweakreg/tests/test_tweakreg.py b/jwst/tweakreg/tests/test_tweakreg.py index c6f6e2b596..3084477bd0 100644 --- a/jwst/tweakreg/tests/test_tweakreg.py +++ b/jwst/tweakreg/tests/test_tweakreg.py @@ -201,6 +201,25 @@ def test_tweakreg_step(example_input, with_shift): assert abs_delta < 1E-12 +@pytest.mark.parametrize("alignment_type", ['', 'abs_']) +def test_src_confusion_pars_test(example_input, alignment_type): + # assign images to different groups (so they are aligned to each other) + example_input[0].meta.group_id = 'a' + example_input[1].meta.group_id = 'b' + + # make the step with arguments that may cause source confusion in match + pars = { + f"{alignment_type}separation": 1.0, + f"{alignment_type}tolerance": 1.0, + } + step = tweakreg_step.TweakRegStep(**pars) + result = step(example_input) + + # check that step was skipped + for model in result: + assert model.meta.cal_step.tweakreg == 'SKIPPED' + + @pytest.fixture() def custom_catalog_path(tmp_path): fn = tmp_path / "custom_catalog.ecsv" @@ -286,7 +305,7 @@ def test_custom_catalog(custom_catalog_path, example_input, catfile, asn, meta, elif catfile == "invalid_catfile": pass - # figure out how many sources to expect for the model in group 'a' + # figure out how many sources to expect for the model in group 'a' n_custom_sources = N_EXAMPLE_SOURCES if custom: if catfile == "valid_catfile": From 2b152b114f09a301f86d6cfb0fed777d254d4a15 Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Sun, 26 May 2024 23:15:00 -0400 Subject: [PATCH 8/9] change unit test name --- jwst/tweakreg/tests/test_tweakreg.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jwst/tweakreg/tests/test_tweakreg.py b/jwst/tweakreg/tests/test_tweakreg.py index 3084477bd0..47b071c337 100644 --- a/jwst/tweakreg/tests/test_tweakreg.py +++ b/jwst/tweakreg/tests/test_tweakreg.py @@ -202,7 +202,7 @@ def test_tweakreg_step(example_input, with_shift): @pytest.mark.parametrize("alignment_type", ['', 'abs_']) -def test_src_confusion_pars_test(example_input, alignment_type): +def test_src_confusion_pars(example_input, alignment_type): # assign images to different groups (so they are aligned to each other) example_input[0].meta.group_id = 'a' example_input[1].meta.group_id = 'b' From 30739c58748c8db0bc7a1d31ace8112f588acbb4 Mon Sep 17 00:00:00 2001 From: Mihai Cara Date: Tue, 4 Jun 2024 15:18:33 -0400 Subject: [PATCH 9/9] fix regression test --- jwst/regtest/test_niriss_image.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jwst/regtest/test_niriss_image.py b/jwst/regtest/test_niriss_image.py index d117b3fd10..4cce41eda6 100644 --- a/jwst/regtest/test_niriss_image.py +++ b/jwst/regtest/test_niriss_image.py @@ -60,9 +60,9 @@ def test_niriss_tweakreg_no_sources(rtdata, fitsdiff_default_kwargs): "--abs_refcat='GAIADR3'", "--save_results=True", ] + + # run the test from the command line: result = Step.from_cmdline(args) - # Check that the step is skipped - assert result.skip # Check the status of the step is set correctly in the files. mc = datamodels.ModelContainer(rtdata.input)