From 8342f2ac2d56cae13dac3f155fa69eb55a425587 Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Wed, 4 Dec 2024 09:53:01 -0500 Subject: [PATCH 1/9] Use private _all_arrays to set --- src/diffpy/utils/diffraction_objects.py | 45 +++++++++++++++---------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/diffpy/utils/diffraction_objects.py b/src/diffpy/utils/diffraction_objects.py index 54374fa7..66b840e5 100644 --- a/src/diffpy/utils/diffraction_objects.py +++ b/src/diffpy/utils/diffraction_objects.py @@ -181,6 +181,17 @@ def __rtruediv__(self, other): divided.on_q[1] = other.on_q[1] / self.on_q[1] return divided + @property + def all_arrays(self): + return self._all_arrays + + @all_arrays.setter + def all_arrays(self, value): + raise AttributeError( + "Direct modification of 'all_arrays' is not allowed." + "Please use 'insert_scattering_quantity' to modify it." + ) + def set_angles_from_list(self, angles_list): self.angles = angles_list self.n_steps = len(angles_list) - 1.0 @@ -259,25 +270,25 @@ def get_angle_index(self, angle): raise IndexError(f"WARNING: no angle {angle} found in angles list") def _set_xarrays(self, xarray, xtype): - self.all_arrays = np.empty(shape=(len(xarray), 4)) + self._all_arrays = np.empty(shape=(len(xarray), 4)) if xtype.lower() in QQUANTITIES: - self.all_arrays[:, 1] = xarray - self.all_arrays[:, 2] = q_to_tth(xarray, self.wavelength) - self.all_arrays[:, 3] = q_to_d(xarray) + self._all_arrays[:, 1] = xarray + self._all_arrays[:, 2] = q_to_tth(xarray, self.wavelength) + self._all_arrays[:, 3] = q_to_d(xarray) elif xtype.lower() in ANGLEQUANTITIES: - self.all_arrays[:, 2] = xarray - self.all_arrays[:, 1] = tth_to_q(xarray, self.wavelength) - self.all_arrays[:, 3] = tth_to_d(xarray, self.wavelength) + self._all_arrays[:, 2] = xarray + self._all_arrays[:, 1] = tth_to_q(xarray, self.wavelength) + self._all_arrays[:, 3] = tth_to_d(xarray, self.wavelength) elif xtype.lower() in DQUANTITIES: - self.all_arrays[:, 3] = xarray - self.all_arrays[:, 1] = d_to_q(xarray) - self.all_arrays[:, 2] = d_to_tth(xarray, self.wavelength) - self.qmin = np.nanmin(self.all_arrays[:, 1], initial=np.inf) - self.qmax = np.nanmax(self.all_arrays[:, 1], initial=0.0) - self.tthmin = np.nanmin(self.all_arrays[:, 2], initial=np.inf) - self.tthmax = np.nanmax(self.all_arrays[:, 2], initial=0.0) - self.dmin = np.nanmin(self.all_arrays[:, 3], initial=np.inf) - self.dmax = np.nanmax(self.all_arrays[:, 3], initial=0.0) + self._all_arrays[:, 3] = xarray + self._all_arrays[:, 1] = d_to_q(xarray) + self._all_arrays[:, 2] = d_to_tth(xarray, self.wavelength) + self.qmin = np.nanmin(self._all_arrays[:, 1], initial=np.inf) + self.qmax = np.nanmax(self._all_arrays[:, 1], initial=0.0) + self.tthmin = np.nanmin(self._all_arrays[:, 2], initial=np.inf) + self.tthmax = np.nanmax(self._all_arrays[:, 2], initial=0.0) + self.dmin = np.nanmin(self._all_arrays[:, 3], initial=np.inf) + self.dmax = np.nanmax(self._all_arrays[:, 3], initial=0.0) def insert_scattering_quantity( self, @@ -309,7 +320,7 @@ def insert_scattering_quantity( """ self._set_xarrays(xarray, xtype) - self.all_arrays[:, 0] = yarray + self._all_arrays[:, 0] = yarray self.input_xtype = xtype # only update these optional values if non-empty quantities are passed to avoid overwriting # valid data inadvertently From 6278a8ebfdd195821e06d8960e65b3c16202ffee Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Wed, 4 Dec 2024 09:53:24 -0500 Subject: [PATCH 2/9] Update tests with actual object info --- tests/test_diffraction_objects.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/test_diffraction_objects.py b/tests/test_diffraction_objects.py index 9db6932e..4651273e 100644 --- a/tests/test_diffraction_objects.py +++ b/tests/test_diffraction_objects.py @@ -341,5 +341,27 @@ def test_dump(tmp_path, mocker): @pytest.mark.parametrize("inputs, expected", tc_params) def test_constructor(inputs, expected): - actualdo = DiffractionObject(**inputs) - compare_dicts(actualdo.__dict__, expected) + actual_do = DiffractionObject(**inputs) + actual_dict = { + "all_arrays": actual_do.all_arrays, + "metadata": actual_do.metadata, + "input_xtype": actual_do.input_xtype, + "name": actual_do.name, + "scat_quantity": actual_do.scat_quantity, + "qmin": actual_do.qmin, + "qmax": actual_do.qmax, + "tthmin": actual_do.tthmin, + "tthmax": actual_do.tthmax, + "dmin": actual_do.dmin, + "dmax": actual_do.dmax, + "wavelength": actual_do.wavelength, + } + compare_dicts(actual_dict, expected) + + +def test_all_array_setter(): + actual_do = DiffractionObject() + + # Attempt to directly modify the property + with pytest.raises(AttributeError, match="Direct modification of 'all_arrays' is not allowed."): + actual_do.all_arrays = np.empty((4, 4)) From 9772e1ce034f2ed51aed218540237edacd056f1e Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Wed, 4 Dec 2024 09:53:33 -0500 Subject: [PATCH 3/9] Add news --- news/setter-property.rst | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 news/setter-property.rst diff --git a/news/setter-property.rst b/news/setter-property.rst new file mode 100644 index 00000000..8b2ddc97 --- /dev/null +++ b/news/setter-property.rst @@ -0,0 +1,23 @@ +**Added:** + +* prevent direct modification of `all_arrays` using `@property` + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* + +**Security:** + +* From 3ba3038f6531a4c0951a193f5f1cf0548085afcc Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Thu, 5 Dec 2024 13:38:39 -0500 Subject: [PATCH 4/9] Use deepdiff to compare do.__dict__ --- news/scattering_obt.rst | 2 -- requirements/test.txt | 1 + tests/test_diffraction_objects.py | 44 ++++++++++++++++++------------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/news/scattering_obt.rst b/news/scattering_obt.rst index 0090b88e..e5177fde 100644 --- a/news/scattering_obt.rst +++ b/news/scattering_obt.rst @@ -19,5 +19,3 @@ * **Security:** - -* diff --git a/requirements/test.txt b/requirements/test.txt index 9966e09d..a392bd23 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -6,3 +6,4 @@ pytest-env pytest-mock pytest-cov freezegun +DeepDiff diff --git a/tests/test_diffraction_objects.py b/tests/test_diffraction_objects.py index 4651273e..c80f15b8 100644 --- a/tests/test_diffraction_objects.py +++ b/tests/test_diffraction_objects.py @@ -2,6 +2,7 @@ import numpy as np import pytest +from deepdiff import DeepDiff from freezegun import freeze_time from diffpy.utils.diffraction_objects import DiffractionObject @@ -248,7 +249,7 @@ def test_dump(tmp_path, mocker): ( {}, { - "all_arrays": np.empty(shape=(0, 4)), # instantiate empty + "_all_arrays": np.empty(shape=(0, 4)), # instantiate empty "metadata": {}, "input_xtype": "", "name": "", @@ -265,7 +266,7 @@ def test_dump(tmp_path, mocker): ( # instantiate just non-array attributes {"name": "test", "scat_quantity": "x-ray", "metadata": {"thing": "1", "another": "2"}}, { - "all_arrays": np.empty(shape=(0, 4)), + "_all_arrays": np.empty(shape=(0, 4)), "metadata": {"thing": "1", "another": "2"}, "input_xtype": "", "name": "test", @@ -287,7 +288,7 @@ def test_dump(tmp_path, mocker): "wavelength": 4.0 * np.pi, }, { - "all_arrays": np.array( + "_all_arrays": np.array( [ [1.0, 0.0, 0.0, np.float64(np.inf)], [2.0, 1.0 / np.sqrt(2), 90.0, np.sqrt(2) * 2 * np.pi], @@ -316,7 +317,7 @@ def test_dump(tmp_path, mocker): "scat_quantity": "x-ray", }, { - "all_arrays": np.array( + "_all_arrays": np.array( [ [1.0, 0.0, 0.0, np.float64(np.inf)], [2.0, 1.0 / np.sqrt(2), 90.0, np.sqrt(2) * 2 * np.pi], @@ -342,21 +343,26 @@ def test_dump(tmp_path, mocker): @pytest.mark.parametrize("inputs, expected", tc_params) def test_constructor(inputs, expected): actual_do = DiffractionObject(**inputs) - actual_dict = { - "all_arrays": actual_do.all_arrays, - "metadata": actual_do.metadata, - "input_xtype": actual_do.input_xtype, - "name": actual_do.name, - "scat_quantity": actual_do.scat_quantity, - "qmin": actual_do.qmin, - "qmax": actual_do.qmax, - "tthmin": actual_do.tthmin, - "tthmax": actual_do.tthmax, - "dmin": actual_do.dmin, - "dmax": actual_do.dmax, - "wavelength": actual_do.wavelength, - } - compare_dicts(actual_dict, expected) + diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=4) + # Ensure there is no difference + assert diff == {} + + +def test_all_array_getter(): + actual_do = DiffractionObject( + xarray=np.array([0.0, 90.0, 180.0]), + yarray=np.array([1.0, 2.0, 3.0]), + xtype="tth", + wavelength=4.0 * np.pi, + ) + expected_all_arrays = np.array( + [ + [1.0, 0.0, 0.0, np.float64(np.inf)], + [2.0, 1.0 / np.sqrt(2), 90.0, np.sqrt(2) * 2 * np.pi], + [3.0, 1.0, 180.0, 1.0 * 2 * np.pi], + ] + ) + assert np.array_equal(actual_do.all_arrays, expected_all_arrays) def test_all_array_setter(): From 71fcb6b366320a7a0f91e9c5c3848f5f8d94c40d Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Thu, 5 Dec 2024 13:41:12 -0500 Subject: [PATCH 5/9] Re-design error msg on how to modify all_arrays --- src/diffpy/utils/diffraction_objects.py | 4 ++-- tests/test_diffraction_objects.py | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/diffpy/utils/diffraction_objects.py b/src/diffpy/utils/diffraction_objects.py index 66b840e5..82ad9cf4 100644 --- a/src/diffpy/utils/diffraction_objects.py +++ b/src/diffpy/utils/diffraction_objects.py @@ -188,8 +188,8 @@ def all_arrays(self): @all_arrays.setter def all_arrays(self, value): raise AttributeError( - "Direct modification of 'all_arrays' is not allowed." - "Please use 'insert_scattering_quantity' to modify it." + "Direct modification of attribute 'all_arrays' is not allowed." + "Please use 'insert_scattering_quantity' to modify `all_arrays`." ) def set_angles_from_list(self, angles_list): diff --git a/tests/test_diffraction_objects.py b/tests/test_diffraction_objects.py index c80f15b8..a484de39 100644 --- a/tests/test_diffraction_objects.py +++ b/tests/test_diffraction_objects.py @@ -369,5 +369,9 @@ def test_all_array_setter(): actual_do = DiffractionObject() # Attempt to directly modify the property - with pytest.raises(AttributeError, match="Direct modification of 'all_arrays' is not allowed."): + with pytest.raises( + AttributeError, + match="Direct modification of attribute 'all_arrays' is not allowed." + "Please use 'insert_scattering_quantity' to modify `all_arrays`.", + ): actual_do.all_arrays = np.empty((4, 4)) From f635f4d8b9c89709ca2728327aa1e5b3e77bae21 Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Thu, 5 Dec 2024 16:26:51 -0500 Subject: [PATCH 6/9] Add sig fig to 13 and remove comment not needed --- tests/test_diffraction_objects.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_diffraction_objects.py b/tests/test_diffraction_objects.py index a484de39..f0c6c2be 100644 --- a/tests/test_diffraction_objects.py +++ b/tests/test_diffraction_objects.py @@ -343,8 +343,7 @@ def test_dump(tmp_path, mocker): @pytest.mark.parametrize("inputs, expected", tc_params) def test_constructor(inputs, expected): actual_do = DiffractionObject(**inputs) - diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=4) - # Ensure there is no difference + diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=13) assert diff == {} From b7fa23e8c850f52421890b5394a21c3f1dd01a08 Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Thu, 5 Dec 2024 16:30:45 -0500 Subject: [PATCH 7/9] Use all close for comparing expected all_array value --- tests/test_diffraction_objects.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_diffraction_objects.py b/tests/test_diffraction_objects.py index f0c6c2be..b7d8eae3 100644 --- a/tests/test_diffraction_objects.py +++ b/tests/test_diffraction_objects.py @@ -361,7 +361,7 @@ def test_all_array_getter(): [3.0, 1.0, 180.0, 1.0 * 2 * np.pi], ] ) - assert np.array_equal(actual_do.all_arrays, expected_all_arrays) + assert np.allclose(actual_do.all_arrays, expected_all_arrays) def test_all_array_setter(): From ba889ece9f9633e9a0d7f9fed108ab9427e5eb84 Mon Sep 17 00:00:00 2001 From: Sangjoon Bob Lee Date: Thu, 5 Dec 2024 16:33:49 -0500 Subject: [PATCH 8/9] Reset scattering_obj news --- news/scattering_obt.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/news/scattering_obt.rst b/news/scattering_obt.rst index e5177fde..e1f0cf53 100644 --- a/news/scattering_obt.rst +++ b/news/scattering_obt.rst @@ -19,3 +19,5 @@ * **Security:** + +* From d20d2bcd97600831f991fea80d7b3a3bb59926bd Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 5 Dec 2024 21:33:58 +0000 Subject: [PATCH 9/9] [pre-commit.ci] auto fixes from pre-commit hooks --- news/scattering_obt.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/scattering_obt.rst b/news/scattering_obt.rst index e1f0cf53..0090b88e 100644 --- a/news/scattering_obt.rst +++ b/news/scattering_obt.rst @@ -20,4 +20,4 @@ **Security:** -* +*