From 28ab0919ca87fe24a540ea4af4c83c8df53c1bd3 Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 14 Sep 2023 14:16:08 -0400 Subject: [PATCH 01/11] use deep diff instead of asdf.commands.diff --- pyproject.toml | 1 + romancal/regtest/conftest.py | 7 +- romancal/regtest/regtestdata.py | 126 ++++++++++++++++++++++++++++++-- 3 files changed, 126 insertions(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 04a7edb9b..0b5736916 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,6 +53,7 @@ test = [ 'pytest >=4.6.0', 'pytest-astropy', 'metrics_logger >= 0.1.0', + 'deepdiff', ] dev = [ 'romancal[docs,test]', diff --git a/romancal/regtest/conftest.py b/romancal/regtest/conftest.py index 3ea368184..06e4987b0 100644 --- a/romancal/regtest/conftest.py +++ b/romancal/regtest/conftest.py @@ -203,10 +203,13 @@ def rtdata_module(artifactory_repos, envopt, request, jail): @pytest.fixture def ignore_asdf_paths(): ignore_attr = [ - "meta.[date, filename]", + "roman.meta.date", + # roman.meta.filenam is used by the ExposurePipeline so should likely + # not be ignored here + # "roman.meta.filename", "asdf_library", "history", - "cal_logs", + "roman.meta.cal_logs", ] return {"ignore": ignore_attr} diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index 449662adb..6fdee5a14 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -5,18 +5,23 @@ import sys from difflib import unified_diff from glob import glob as _sys_glob -from io import StringIO from pathlib import Path import asdf +import astropy.time +import deepdiff +import gwcs +import numpy as np import requests -from asdf.commands.diff import diff as asdf_diff +from astropy.units import Quantity from ci_watson.artifactory_helpers import ( BigdataError, check_url, get_bigdata, get_bigdata_root, ) +from deepdiff.operator import BaseOperator +from gwcs.converters.tests.test_wcs import _assert_wcs_equal # from romancal.lib.suffix import replace_suffix from romancal.stpipe import RomanStep @@ -525,8 +530,117 @@ def _data_glob_url(*url_parts, root=None): return url_paths +class NDArrayTypeOperator(BaseOperator): + def __init__(self, rtol=1e-05, atol=1e-08, equal_nan=True, **kwargs): + super().__init__(**kwargs) + self.rtol = rtol + self.atol = atol + self.equal_nan = equal_nan + + def give_up_diffing(self, level, diff_instance): + a, b = level.t1, level.t2 + meta = {} + if a.shape != b.shape: + meta["shapes"] = [a.shape, b.shape] + if a.dtype != b.dtype: + meta["dtypes"] = [a.dtype, b.dtype] + if isinstance(a, Quantity) and isinstance(b, Quantity): + if a.unit != b.unit: + meta["units"] = [a.unit, b.unit] + if not meta: # only compare if shapes and dtypes match + if not np.allclose( + a, b, rtol=self.rtol, atol=self.atol, equal_nan=self.equal_nan + ): + abs_diff = np.abs(a - b) + index = np.unravel_index(np.nanargmax(abs_diff), a.shape) + meta["worst_abs_diff"] = { + "index": index, + "value": abs_diff[index], + } + with np.errstate(invalid="ignore", divide="ignore"): + # 0 / 0 == nan and produces an 'invalid' error + # 1 / 0 == inf and produces a 'divide' error + # ignore these here for computing the fractional diff + fractional_diff = np.abs(a / b) + index = np.unravel_index(np.nanargmax(fractional_diff), a.shape) + meta["worst_fractional_diff"] = { + "index": index, + "value": fractional_diff[index], + } + meta["abs_diff"] = np.nansum(np.abs(a - b)) + meta["n_diffs"] = np.count_nonzero( + np.isclose( + a, b, rtol=self.rtol, atol=self.atol, equal_nan=self.equal_nan + ) + ) + if meta: + diff_instance.custom_report_result("arrays_differ", level, meta) + return True + + +class TimeOperator(BaseOperator): + def give_up_diffing(self, level, diff_instance): + if level.t1 != level.t2: + diff_instance.custom_report_result( + "times_differ", + level, + { + "difference": level.t1 - level.t2, + }, + ) + return True + + +def wcs_equal(a, b): + try: + # can this be made part of the public gwcs api? + _assert_wcs_equal(a, b) + return True + except AssertionError: + # TODO return information about difference + return False + + +class WCSOperator(BaseOperator): + def give_up_diffing(self, level, diff_instance): + if not wcs_equal(level.t1, level.t2): + diff_instance.custom_report_result( + "wcs_differ", + level, + { + "extra": "information", + }, + ) + return True + + def compare_asdf(result, truth, **kwargs): - f = StringIO() - asdf_diff([result, truth], minimal=False, iostream=f, **kwargs) - if f.getvalue(): - f.getvalue() + exclude_paths = [] + for path in kwargs.get("ignore", []): + key_path = "".join([f"['{k}']" for k in path.split(".")]) + exclude_paths.append(f"root{key_path}") + ndarray_kwargs = {} + for k in ("rtol", "atol", "equal_nan"): + if k in kwargs: + ndarray_kwargs[k] = kwargs[k] + operators = [ + NDArrayTypeOperator( + types=[asdf.tags.core.NDArrayType, np.ndarray], **ndarray_kwargs + ), + TimeOperator(types=[astropy.time.Time]), + WCSOperator(types=[gwcs.WCS]), + ] + with asdf.open(result) as af0, asdf.open(truth) as af1: + diff = deepdiff.DeepDiff( + af0.tree, + af1.tree, + ignore_nan_inequality=kwargs.get("equal_nan", False), + custom_operators=operators, + exclude_paths=exclude_paths, + ) + # the conversion between NDArrayType and ndarray adds a bunch + # of type changes, ignore these for now. + # TODO Ideally we could find a way to remove just the NDArrayType ones + if "type_changes" in diff: + del diff["type_changes"] + return diff or None From 9d512d05c0c59a548b597324cc47e9049464cc7e Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 14 Sep 2023 16:59:27 -0400 Subject: [PATCH 02/11] enable verbose pytest output in jenkins tests --- JenkinsfileRT | 3 +++ JenkinsfileRT_dev | 3 +++ 2 files changed, 6 insertions(+) diff --git a/JenkinsfileRT b/JenkinsfileRT index 4cd838b68..48824c0b3 100644 --- a/JenkinsfileRT +++ b/JenkinsfileRT @@ -96,8 +96,11 @@ bc0.build_cmds = bc0.build_cmds + PipInject(env.OVERRIDE_REQUIREMENTS) bc0.build_cmds = bc0.build_cmds + [ "pip list" ] +// Using -v below is important for seeing the full output for regtests +// that fail a comparison with a truth file bc0.test_cmds = [ "pytest --cov-report=xml:coverage.xml --cov=./ -r sxf -n auto --bigdata --slow \ + -v \ --ddtrace \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", diff --git a/JenkinsfileRT_dev b/JenkinsfileRT_dev index 787c910dc..f40e97152 100644 --- a/JenkinsfileRT_dev +++ b/JenkinsfileRT_dev @@ -90,8 +90,11 @@ bc0.build_cmds = bc0.build_cmds + PipInject(env.OVERRIDE_REQUIREMENTS) bc0.build_cmds = bc0.build_cmds + [ "pip list" ] +// Using -v below is important for seeing the full output for regtests +// that fail a comparison with a truth file bc0.test_cmds = [ "pytest -r sxf -n 0 --bigdata --slow \ + -v \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", ] From 253e284adba8a5910a3d43d13692f5375dab91e3 Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 14 Sep 2023 17:03:12 -0400 Subject: [PATCH 03/11] update changelog --- CHANGES.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 48b409dd0..2d760a1bb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,6 +10,10 @@ general - Fix regression tests for PSF fitting methods [#872] + - Fix regression test ``compare_asdf`` function replacing use of + ``asdf.commands.diff`` with ``deepdiff`` and add ``deepdiff` as + a test dependency [#868] + ramp_fitting ------------ From 47b3a78bab5901f07ad6b9afbb9f04883d468a36 Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 14 Sep 2023 17:18:08 -0400 Subject: [PATCH 04/11] change wcs test to evaluate ra/dec --- romancal/regtest/regtestdata.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index 6fdee5a14..c9398f0c0 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -21,7 +21,7 @@ get_bigdata_root, ) from deepdiff.operator import BaseOperator -from gwcs.converters.tests.test_wcs import _assert_wcs_equal +from gwcs.wcstools import grid_from_bounding_box # from romancal.lib.suffix import replace_suffix from romancal.stpipe import RomanStep @@ -591,25 +591,30 @@ def give_up_diffing(self, level, diff_instance): return True -def wcs_equal(a, b): - try: - # can this be made part of the public gwcs api? - _assert_wcs_equal(a, b) - return True - except AssertionError: - # TODO return information about difference - return False +def _wcs_to_ra_dec(wcs): + x, y = grid_from_bounding_box(wcs.bounding_box) + return wcs(x, y) class WCSOperator(BaseOperator): def give_up_diffing(self, level, diff_instance): - if not wcs_equal(level.t1, level.t2): + # for comparing wcs instances this function evaluates + # each wcs and compares the resulting ra and dec outputs + # TODO should we compare the bounding boxes? + ra_a, dec_a = _wcs_to_ra_dec(level.t1) + ra_b, dec_b = _wcs_to_ra_dec(level.t2) + meta = {} + for name, a, b in [("ra", ra_a, ra_b), ("dec", dec_a, dec_b)]: + # TODO do we want to do something fancier than allclose? + if not np.allclose(a, b): + meta[name] = { + "abs_diff": np.abs(a - b), + } + if meta: diff_instance.custom_report_result( "wcs_differ", level, - { - "extra": "information", - }, + meta, ) return True From ecb43e7dabc26f990e7d5962582d1bc2fde6dddb Mon Sep 17 00:00:00 2001 From: Brett Date: Thu, 14 Sep 2023 17:21:05 -0400 Subject: [PATCH 05/11] disable lazy loading and memmaping for asdf comparison --- romancal/regtest/regtestdata.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index c9398f0c0..c9bb80a78 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -635,7 +635,18 @@ def compare_asdf(result, truth, **kwargs): TimeOperator(types=[astropy.time.Time]), WCSOperator(types=[gwcs.WCS]), ] - with asdf.open(result) as af0, asdf.open(truth) as af1: + # warnings can be seen in regtest runs which indicate + # that ddtrace logs are evaluated at times after the below + # with statement exits resulting in access attempts on the + # closed asdf file. To try and avoid that we disable + # lazy loading and memmory mapping + open_kwargs = { + "lazy_load": False, + "copy_arrays": True, + } + with asdf.open(result, **open_kwargs) as af0, asdf.open( + truth, **open_kwargs + ) as af1: diff = deepdiff.DeepDiff( af0.tree, af1.tree, From 7c0086c760761f11e82e47d733d94b0536dffa65 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 15 Sep 2023 10:30:00 -0400 Subject: [PATCH 06/11] document compare_asdf, pretty print output disable pytest verbose output in jenkins files as pretty print output should appear in test summaries --- JenkinsfileRT | 3 -- JenkinsfileRT_dev | 3 -- romancal/regtest/regtestdata.py | 49 +++++++++++++++++++++++++++------ 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/JenkinsfileRT b/JenkinsfileRT index 48824c0b3..4cd838b68 100644 --- a/JenkinsfileRT +++ b/JenkinsfileRT @@ -96,11 +96,8 @@ bc0.build_cmds = bc0.build_cmds + PipInject(env.OVERRIDE_REQUIREMENTS) bc0.build_cmds = bc0.build_cmds + [ "pip list" ] -// Using -v below is important for seeing the full output for regtests -// that fail a comparison with a truth file bc0.test_cmds = [ "pytest --cov-report=xml:coverage.xml --cov=./ -r sxf -n auto --bigdata --slow \ - -v \ --ddtrace \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", diff --git a/JenkinsfileRT_dev b/JenkinsfileRT_dev index f40e97152..787c910dc 100644 --- a/JenkinsfileRT_dev +++ b/JenkinsfileRT_dev @@ -90,11 +90,8 @@ bc0.build_cmds = bc0.build_cmds + PipInject(env.OVERRIDE_REQUIREMENTS) bc0.build_cmds = bc0.build_cmds + [ "pip list" ] -// Using -v below is important for seeing the full output for regtests -// that fail a comparison with a truth file bc0.test_cmds = [ "pytest -r sxf -n 0 --bigdata --slow \ - -v \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", ] diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index c9bb80a78..8498ede56 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -619,18 +619,45 @@ def give_up_diffing(self, level, diff_instance): return True -def compare_asdf(result, truth, **kwargs): +def compare_asdf( + result, truth, ignore=None, rtol=1e-05, atol=1e-08, equal_nan=True, pprint_diff=True +): + """ + Compare 2 asdf files: result and truth. Note that this comparison is + asymmetric (swapping result and truth will give a different result). + + Parameters + ---------- + + result : str + Filename of result asdf file + + truth : str + Filename of truth asdf file + + ignore : list + List of tree node paths to ignore during the comparison + + rtol : float + rtol argument passed to `numpyp.allclose` + + atol : float + atol argument passed to `numpyp.allclose` + + equal_nan : bool + Ignore nan inequality + + pprint : bool + pretty print diff output if a difference is found + """ exclude_paths = [] - for path in kwargs.get("ignore", []): + ignore = ignore or [] + for path in ignore: key_path = "".join([f"['{k}']" for k in path.split(".")]) exclude_paths.append(f"root{key_path}") - ndarray_kwargs = {} - for k in ("rtol", "atol", "equal_nan"): - if k in kwargs: - ndarray_kwargs[k] = kwargs[k] operators = [ NDArrayTypeOperator( - types=[asdf.tags.core.NDArrayType, np.ndarray], **ndarray_kwargs + rtol, atol, equal_nan, types=[asdf.tags.core.NDArrayType, np.ndarray] ), TimeOperator(types=[astropy.time.Time]), WCSOperator(types=[gwcs.WCS]), @@ -650,7 +677,7 @@ def compare_asdf(result, truth, **kwargs): diff = deepdiff.DeepDiff( af0.tree, af1.tree, - ignore_nan_inequality=kwargs.get("equal_nan", False), + ignore_nan_inequality=equal_nan, custom_operators=operators, exclude_paths=exclude_paths, ) @@ -659,4 +686,8 @@ def compare_asdf(result, truth, **kwargs): # TODO Ideally we could find a way to remove just the NDArrayType ones if "type_changes" in diff: del diff["type_changes"] - return diff or None + if not diff: + return None + if pprint_diff: + pprint.pprint(diff) + return diff From a63ff45b097f6fea03699a87ac0b98b5be0f30cc Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 15 Sep 2023 10:49:13 -0400 Subject: [PATCH 07/11] ignore cal_logs not meta.cal_logs --- romancal/regtest/conftest.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/romancal/regtest/conftest.py b/romancal/regtest/conftest.py index 06e4987b0..742a319b3 100644 --- a/romancal/regtest/conftest.py +++ b/romancal/regtest/conftest.py @@ -203,13 +203,16 @@ def rtdata_module(artifactory_repos, envopt, request, jail): @pytest.fixture def ignore_asdf_paths(): ignore_attr = [ + # generic asdf stuff that will contain program version numbers + # and other things that will almost certainly change in every case + "asdf_library", + "history", + # roman-specific stuff to ignore + "roman.cal_logs", "roman.meta.date", - # roman.meta.filenam is used by the ExposurePipeline so should likely + # roman.meta.filename is used by the ExposurePipeline so should likely # not be ignored here # "roman.meta.filename", - "asdf_library", - "history", - "roman.meta.cal_logs", ] return {"ignore": ignore_attr} From 1acf8f6c7a2547440ef3cd3ea18f8460a9d51827 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 15 Sep 2023 11:05:18 -0400 Subject: [PATCH 08/11] re-enable pytest verbose output in jenkins files --- JenkinsfileRT | 1 + JenkinsfileRT_dev | 1 + 2 files changed, 2 insertions(+) diff --git a/JenkinsfileRT b/JenkinsfileRT index 4cd838b68..11f11f326 100644 --- a/JenkinsfileRT +++ b/JenkinsfileRT @@ -98,6 +98,7 @@ bc0.build_cmds = bc0.build_cmds + [ ] bc0.test_cmds = [ "pytest --cov-report=xml:coverage.xml --cov=./ -r sxf -n auto --bigdata --slow \ + -v \ --ddtrace \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", diff --git a/JenkinsfileRT_dev b/JenkinsfileRT_dev index 787c910dc..e9abde3f8 100644 --- a/JenkinsfileRT_dev +++ b/JenkinsfileRT_dev @@ -92,6 +92,7 @@ bc0.build_cmds = bc0.build_cmds + [ ] bc0.test_cmds = [ "pytest -r sxf -n 0 --bigdata --slow \ + -v \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", ] From 7caecc1edf6d4aaacdada77d73027cdfa173e02f Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 15 Sep 2023 16:39:27 -0400 Subject: [PATCH 09/11] change compare_asdf to return DiffResult instance --- JenkinsfileRT | 1 - JenkinsfileRT_dev | 1 - romancal/regtest/regtestdata.py | 29 ++++++++++++++++--------- romancal/regtest/test_dark_current.py | 12 ++++++---- romancal/regtest/test_jump_det.py | 3 ++- romancal/regtest/test_linearity.py | 6 +++-- romancal/regtest/test_ramp_fitting.py | 6 +++-- romancal/regtest/test_refpix.py | 3 ++- romancal/regtest/test_tweakreg.py | 6 ++--- romancal/regtest/test_wfi_dq_init.py | 10 +++++---- romancal/regtest/test_wfi_flat_field.py | 16 +++++++++----- romancal/regtest/test_wfi_photom.py | 5 +++-- romancal/regtest/test_wfi_pipeline.py | 15 ++++++++----- romancal/regtest/test_wfi_saturation.py | 6 +++-- 14 files changed, 75 insertions(+), 44 deletions(-) diff --git a/JenkinsfileRT b/JenkinsfileRT index 11f11f326..4cd838b68 100644 --- a/JenkinsfileRT +++ b/JenkinsfileRT @@ -98,7 +98,6 @@ bc0.build_cmds = bc0.build_cmds + [ ] bc0.test_cmds = [ "pytest --cov-report=xml:coverage.xml --cov=./ -r sxf -n auto --bigdata --slow \ - -v \ --ddtrace \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", diff --git a/JenkinsfileRT_dev b/JenkinsfileRT_dev index e9abde3f8..787c910dc 100644 --- a/JenkinsfileRT_dev +++ b/JenkinsfileRT_dev @@ -92,7 +92,6 @@ bc0.build_cmds = bc0.build_cmds + [ ] bc0.test_cmds = [ "pytest -r sxf -n 0 --bigdata --slow \ - -v \ --basetemp=${pytest_basetemp} --junit-xml=results.xml --dist=loadscope \ --env=${artifactoryenv} ${pytest_args}", ] diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index 8498ede56..10e19ea85 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -619,9 +619,19 @@ def give_up_diffing(self, level, diff_instance): return True -def compare_asdf( - result, truth, ignore=None, rtol=1e-05, atol=1e-08, equal_nan=True, pprint_diff=True -): +class DiffResult: + def __init__(self, diff): + self.diff = diff + + @property + def identical(self): + return not self.diff + + def report(self, **kwargs): + return pprint.pformat(self.diff) + + +def compare_asdf(result, truth, ignore=None, rtol=1e-05, atol=1e-08, equal_nan=True): """ Compare 2 asdf files: result and truth. Note that this comparison is asymmetric (swapping result and truth will give a different result). @@ -647,8 +657,11 @@ def compare_asdf( equal_nan : bool Ignore nan inequality - pprint : bool - pretty print diff output if a difference is found + Returns + ------- + + diff_result : DiffResult + result of the comparison """ exclude_paths = [] ignore = ignore or [] @@ -686,8 +699,4 @@ def compare_asdf( # TODO Ideally we could find a way to remove just the NDArrayType ones if "type_changes" in diff: del diff["type_changes"] - if not diff: - return None - if pprint_diff: - pprint.pprint(diff) - return diff + return DiffResult(diff) diff --git a/romancal/regtest/test_dark_current.py b/romancal/regtest/test_dark_current.py index 00c3177bd..67fcfc9dd 100644 --- a/romancal/regtest/test_dark_current.py +++ b/romancal/regtest/test_dark_current.py @@ -20,7 +20,8 @@ def test_dark_current_subtraction_step(rtdata, ignore_asdf_paths): output = "r0000101001001001001_01101_0001_WFI01_darkcurrent.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -40,7 +41,8 @@ def test_dark_current_outfile_step(rtdata, ignore_asdf_paths): output = "Test_darkcurrent.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -61,7 +63,8 @@ def test_dark_current_outfile_suffix(rtdata, ignore_asdf_paths): output = "Test_darkcurrent.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -83,4 +86,5 @@ def test_dark_current_output(rtdata, ignore_asdf_paths): output = "r0000101001001001001_01101_0001_WFI01_darkcurrent.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_jump_det.py b/romancal/regtest/test_jump_det.py index ae753ba72..ae67c2756 100644 --- a/romancal/regtest/test_jump_det.py +++ b/romancal/regtest/test_jump_det.py @@ -28,4 +28,5 @@ def test_jump_detection_step(rtdata, ignore_asdf_paths): output = "r0000101001001001001_01101_0001_WFI01_jump.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_linearity.py b/romancal/regtest/test_linearity.py index cf5d4a7c3..f58343f45 100644 --- a/romancal/regtest/test_linearity.py +++ b/romancal/regtest/test_linearity.py @@ -18,7 +18,8 @@ def test_linearity_step(rtdata, ignore_asdf_paths): output = "r0000101001001001001_01101_0001_WFI01_linearity.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -34,4 +35,5 @@ def test_linearity_outfile_step(rtdata, ignore_asdf_paths): output = "Test_linearity.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_ramp_fitting.py b/romancal/regtest/test_ramp_fitting.py index 309fee688..ed561e00f 100644 --- a/romancal/regtest/test_ramp_fitting.py +++ b/romancal/regtest/test_ramp_fitting.py @@ -24,9 +24,11 @@ def test_ramp_fitting_step(rtdata, ignore_asdf_paths): output = "r0000101001001001001_01101_0001_WFI01_rampfit.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() output = "rampfit_opt_fitopt.asdf" rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_refpix.py b/romancal/regtest/test_refpix.py index d75988adb..63675eb59 100644 --- a/romancal/regtest/test_refpix.py +++ b/romancal/regtest/test_refpix.py @@ -21,4 +21,5 @@ def test_refpix_step(rtdata, ignore_asdf_paths): rtdata.output = output rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_tweakreg.py b/romancal/regtest/test_tweakreg.py index b96274a8d..d532d9bb8 100644 --- a/romancal/regtest/test_tweakreg.py +++ b/romancal/regtest/test_tweakreg.py @@ -93,8 +93,8 @@ def test_tweakreg(rtdata, ignore_asdf_paths, tmp_path): ) assert "v2v3corr" in tweakreg_out.meta.wcs.available_frames + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( - "DMS280 MSG: Was the proper TweakReg data produced?" - f" : {(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f"DMS280 MSG: Was the proper TweakReg data produced? : {diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_wfi_dq_init.py b/romancal/regtest/test_wfi_dq_init.py index 1075fb2a3..722022b52 100644 --- a/romancal/regtest/test_wfi_dq_init.py +++ b/romancal/regtest/test_wfi_dq_init.py @@ -58,12 +58,13 @@ def test_dq_init_image_step(rtdata, ignore_asdf_paths): assert "roman.pixeldq" in ramp_out.to_flat_dict() rtdata.get_truth(f"truth/WFI/image/{output}") + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( "DMS25 MSG: Was the proper data quality array initialized" " for the ramp data produced? : " - f"{(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f"{diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() @metrics_logger("DMS26") @@ -113,9 +114,10 @@ def test_dq_init_grism_step(rtdata, ignore_asdf_paths): assert "roman.pixeldq" in ramp_out.to_flat_dict() rtdata.get_truth(f"truth/WFI/grism/{output}") + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( "DMS26 MSG: Was proper data quality initialized " "ramp data produced? : " - f"{(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f"{diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_wfi_flat_field.py b/romancal/regtest/test_wfi_flat_field.py index 691799113..6a75cd2e6 100644 --- a/romancal/regtest/test_wfi_flat_field.py +++ b/romancal/regtest/test_wfi_flat_field.py @@ -31,7 +31,8 @@ def test_flat_field_image_step(rtdata, ignore_asdf_paths): args = ["romancal.step.FlatFieldStep", rtdata.input] RomanStep.from_cmdline(args) rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -64,7 +65,8 @@ def test_flat_field_grism_step(rtdata, ignore_asdf_paths): args = ["romancal.step.FlatFieldStep", rtdata.input] RomanStep.from_cmdline(args) rtdata.get_truth(f"truth/WFI/grism/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @metrics_logger("DMS79") @@ -113,12 +115,13 @@ def test_flat_field_crds_match_image_step(rtdata, ignore_asdf_paths): RomanStep.from_cmdline(args) rtdata.get_truth(f"truth/WFI/image/{output}") + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( "DMS79 MSG: Was proper flat fielded " "Level 2 data produced? : " - f"{(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f"{diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() # This test requires a second file, in order to meet the DMS79 requirement. # The test will show that two files with different observation dates match @@ -158,12 +161,13 @@ def test_flat_field_crds_match_image_step(rtdata, ignore_asdf_paths): ) RomanStep.from_cmdline(args) rtdata.get_truth(f"truth/WFI/image/{output}") + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( "DMS79 MSG: Was proper flat fielded " "Level 2 data produced? : " - f"{(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f"{diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() # Test differing flat matches step.log.info( diff --git a/romancal/regtest/test_wfi_photom.py b/romancal/regtest/test_wfi_photom.py index 95a6cbdc3..48916d4e7 100644 --- a/romancal/regtest/test_wfi_photom.py +++ b/romancal/regtest/test_wfi_photom.py @@ -190,8 +190,9 @@ def test_absolute_photometric_calibration(rtdata, ignore_asdf_paths): ) rtdata.get_truth(f"truth/WFI/image/{output}") + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) step.log.info( "DMS140 MSG: Was the proper absolute photometry calibrated image data produced?" - f" : {(compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None)}" + f" : {diff.identical}" ) - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + assert diff.identical, diff.report() diff --git a/romancal/regtest/test_wfi_pipeline.py b/romancal/regtest/test_wfi_pipeline.py index 8fd470868..13dd40a44 100644 --- a/romancal/regtest/test_wfi_pipeline.py +++ b/romancal/regtest/test_wfi_pipeline.py @@ -40,7 +40,8 @@ def test_level2_image_processing_pipeline(rtdata, ignore_asdf_paths): ] ExposurePipeline.from_cmdline(args) rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() # Perform DMS tests # Initial prep @@ -238,7 +239,8 @@ def test_level2_image_processing_pipeline(rtdata, ignore_asdf_paths): # Test that repointed file matches truth rtdata.get_truth("truth/WFI/image/" + output.rsplit(".", 1)[0] + "_repoint.asdf") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() pipeline.log.info( "DMS89 MSG: Testing that the different pointings create differing wcs......." @@ -273,7 +275,8 @@ def test_level2_grism_processing_pipeline(rtdata, ignore_asdf_paths): ] ExposurePipeline.from_cmdline(args) rtdata.get_truth(f"truth/WFI/grism/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() # Perform DMS tests # Initial prep @@ -414,7 +417,8 @@ def test_level2_grism_processing_pipeline(rtdata, ignore_asdf_paths): # Test that repointed file matches truth rtdata.get_truth("truth/WFI/grism/" + output.rsplit(".", 1)[0] + "_repoint.asdf") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() pipeline.log.info( "DMS93 MSG: Testing that the different pointings create differing wcs......." @@ -448,7 +452,8 @@ def test_processing_pipeline_all_saturated(rtdata, ignore_asdf_paths): ExposurePipeline.from_cmdline(args) rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() # Ensure step completion is as expected model = rdm.open(rtdata.output) diff --git a/romancal/regtest/test_wfi_saturation.py b/romancal/regtest/test_wfi_saturation.py index 5c647af63..a45969371 100644 --- a/romancal/regtest/test_wfi_saturation.py +++ b/romancal/regtest/test_wfi_saturation.py @@ -39,7 +39,8 @@ def test_saturation_image_step(rtdata, ignore_asdf_paths): assert "roman.pixeldq" in ramp_out.to_flat_dict() rtdata.get_truth(f"truth/WFI/image/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() @pytest.mark.bigdata @@ -71,4 +72,5 @@ def test_saturation_grism_step(rtdata, ignore_asdf_paths): assert "roman.pixeldq" in ramp_out.to_flat_dict() rtdata.get_truth(f"truth/WFI/grism/{output}") - assert compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) is None + diff = compare_asdf(rtdata.output, rtdata.truth, **ignore_asdf_paths) + assert diff.identical, diff.report() From 2cd83e98306c022260b5b12b79dd7a33b5d2d0b5 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 15 Sep 2023 17:16:42 -0400 Subject: [PATCH 10/11] swap inputs to DeepDiff --- romancal/regtest/regtestdata.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/romancal/regtest/regtestdata.py b/romancal/regtest/regtestdata.py index 10e19ea85..b24e14157 100644 --- a/romancal/regtest/regtestdata.py +++ b/romancal/regtest/regtestdata.py @@ -687,9 +687,12 @@ def compare_asdf(result, truth, ignore=None, rtol=1e-05, atol=1e-08, equal_nan=T with asdf.open(result, **open_kwargs) as af0, asdf.open( truth, **open_kwargs ) as af1: + # swap the inputs here so DeepDiff(truth, result) + # this will create output with 'new_value' referring to + # the value in the result and 'old_value' referring to the truth diff = deepdiff.DeepDiff( - af0.tree, af1.tree, + af0.tree, ignore_nan_inequality=equal_nan, custom_operators=operators, exclude_paths=exclude_paths, From 9f703eba422fbe1d090e542ff7ee13769f42cca6 Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 20 Sep 2023 11:55:33 -0400 Subject: [PATCH 11/11] add initial tests for compare_asdf --- CHANGES.rst | 4 ++-- romancal/regtest/test_regtestdata.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 romancal/regtest/test_regtestdata.py diff --git a/CHANGES.rst b/CHANGES.rst index 2d760a1bb..d151233a3 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,8 +10,8 @@ general - Fix regression tests for PSF fitting methods [#872] - - Fix regression test ``compare_asdf`` function replacing use of - ``asdf.commands.diff`` with ``deepdiff`` and add ``deepdiff` as +- Fix regression test ``compare_asdf`` function replacing use of + ``asdf.commands.diff`` with ``deepdiff`` and add ``deepdiff`` as a test dependency [#868] ramp_fitting diff --git a/romancal/regtest/test_regtestdata.py b/romancal/regtest/test_regtestdata.py new file mode 100644 index 000000000..1e73479e0 --- /dev/null +++ b/romancal/regtest/test_regtestdata.py @@ -0,0 +1,27 @@ +from roman_datamodels import datamodels as rdm +from roman_datamodels import maker_utils + +from romancal.regtest.regtestdata import compare_asdf + + +def test_compare_asdf_identical(tmp_path): + fn0 = tmp_path / "test0.asdf" + fn1 = tmp_path / "test1.asdf" + l2 = rdm.ImageModel(maker_utils.mk_level2_image(shape=(100, 100))) + l2.save(fn0) + l2.save(fn1) + diff = compare_asdf(fn0, fn1) + assert diff.identical, diff.report() + + +def test_compare_asdf_differ(tmp_path): + fn0 = tmp_path / "test0.asdf" + fn1 = tmp_path / "test1.asdf" + l2 = rdm.ImageModel(maker_utils.mk_level2_image(shape=(100, 100))) + l2.save(fn0) + l2.data += 1 * l2.data.unit + l2.save(fn1) + diff = compare_asdf(fn0, fn1) + assert not diff.identical, diff.report() + assert "arrays_differ" in diff.diff + assert "root['roman']['data']" in diff.diff["arrays_differ"]