Skip to content

Use @property to prevent direct modification of all_arrays #196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/setter-property.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* prevent direct modification of `all_arrays` using `@property`

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
1 change: 1 addition & 0 deletions requirements/test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ pytest-env
pytest-mock
pytest-cov
freezegun
DeepDiff
45 changes: 28 additions & 17 deletions src/diffpy/utils/diffraction_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A private property _all_arrays is used internally.


@all_arrays.setter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the user attempts to modify all_array, throw an exception. I've thought it might be better to use exception rather than warning since our _set_xarrays is doing some heavy lifting work and we want our users to use the way we want them to do?

def all_arrays(self, value):
raise AttributeError(
"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):
self.angles = angles_list
self.n_steps = len(angles_list) - 1.0
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
43 changes: 37 additions & 6 deletions tests/test_diffraction_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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": "",
Expand All @@ -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",
Expand All @@ -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],
Expand Down Expand Up @@ -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],
Expand All @@ -341,5 +342,35 @@ 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a library DeepDiff to compare two dicts instead? We've manually defined compare_dicts and dicts_equal within test_diffraction_objects.py which can be replaced using DeepDiff across all .py files.

actual_do = DiffractionObject(**inputs)
diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=13)
assert diff == {}


def test_all_array_getter():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test getter for all_arrays

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.allclose(actual_do.all_arrays, expected_all_arrays)


def test_all_array_setter():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for throwing an exception when the user attempts to modify all_arrays directly.

actual_do = DiffractionObject()

# Attempt to directly modify the property
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))
Loading