Skip to content

Check valid x and y array lenghts and xtypes in insert_scattering_quantity #209

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.

Sign up for GitHub

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 10 commits into from
Dec 12, 2024
Merged

Check valid x and y array lenghts and xtypes in insert_scattering_quantity #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ee190b9
valid xtype and use getter/setting for xtype
bobleesj Dec 8, 2024
c119b1f
Add tests for getter and setter
bobleesj Dec 8, 2024
8e993a3
Add news
bobleesj Dec 8, 2024
f42d40a
Merge branch 'main' into scattering-obj-valid
bobleesj Dec 9, 2024
8431fe2
[pre-commit.ci] auto fixes from pre-commit hooks
pre-commit-ci[bot] Dec 9, 2024
b30cc7e
maintain input_xtype, while make it non-settable
bobleesj Dec 9, 2024
2a7428c
Use _input_xtype within method
bobleesj Dec 9, 2024
c580d82
Add test for mismatch for y and xarray lenght
bobleesj Dec 9, 2024
85290a9
Remove f in the beginning of docstring
bobleesj Dec 9, 2024
9bf5329
Add empty space to _xtype_wmsg
bobleesj Dec 9, 2024
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/scattering-obj-valid.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* validate xtype belongs to XQUANTITIES during DiffractionObject init

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
61 changes: 44 additions & 17 deletions src/diffpy/utils/diffraction_objects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,15 @@

def _xtype_wmsg(xtype):
return (
f"I don't know how to handle the xtype, '{xtype}'. Please rerun specifying an "
f"xtype from {*XQUANTITIES, }"
f"I don't know how to handle the xtype, '{xtype}'. "
f"Please rerun specifying an xtype from {*XQUANTITIES, }"
)


def _setter_wmsg(attribute):
return (
f"Direct modification of attribute '{attribute}' is not allowed. "
f"Please use 'insert_scattering_quantity' to modify '{attribute}'.",
)


Expand All @@ -45,6 +52,7 @@ def __init__(
xarray = np.empty(0)
if yarray is None:
yarray = np.empty(0)

self.insert_scattering_quantity(xarray, yarray, xtype)

def __eq__(self, other):
Expand Down Expand Up @@ -186,11 +194,16 @@ def all_arrays(self):
return self._all_arrays

@all_arrays.setter
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 all_arrays(self, _):
raise AttributeError(_setter_wmsg("all_arrays"))

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we decide that we don't want as setter for input_xtype? Sorry, I am getting a bit confused here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,

    @input_xtype.setter
    def input_xtype(self, _):
        raise AttributeError(_setter_wmsg("input_xtype"))

This code basically, gives AttributeError when the user attempts to modify input_xtype manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. This is what I am seeing on my screen which is why I thought otherwise, but it may not be showing the latest code on GH?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

not looking far enough down. Thanks for the explanation

def input_xtype(self):
return self._input_xtype

@input_xtype.setter
def input_xtype(self, _):
raise AttributeError(_setter_wmsg("input_xtype"))

def set_angles_from_list(self, angles_list):
self.angles = angles_list
Expand Down Expand Up @@ -261,7 +274,7 @@ def _set_array_from_range(self, begin, end, step_size=None, n_steps=None):

def get_array_index(self, value, xtype=None):
"""
returns the index of the closest value in the array associated with the specified xtype
Return the index of the closest value in the array associated with the specified xtype.

Parameters
----------
Expand All @@ -276,7 +289,7 @@ def get_array_index(self, value, xtype=None):
"""

if xtype is None:
xtype = self.input_xtype
xtype = self._input_xtype
array = self.on_xtype(xtype)[0]
if len(array) == 0:
raise ValueError(f"The '{xtype}' array is empty. Please ensure it is initialized.")
Expand Down Expand Up @@ -333,9 +346,18 @@ def insert_scattering_quantity(
Nothing. Updates the object in place.

"""

# Check xarray and yarray have the same length
if len(xarray) != len(yarray):
raise ValueError(
"'xarray' and 'yarray' must have the same length. "
"Please re-initialize 'DiffractionObject' or re-run the method 'insert_scattering_quantity' "
"with 'xarray' and 'yarray' of identical length."
)

self._set_xarrays(xarray, xtype)
self._all_arrays[:, 0] = yarray
self.input_xtype = xtype
self._input_xtype = xtype
# only update these optional values if non-empty quantities are passed to avoid overwriting
# valid data inadvertently
if metadata:
Expand All @@ -347,12 +369,17 @@ def insert_scattering_quantity(
if wavelength is not None:
self.wavelength = wavelength

# Check xtype is valid. An empty string is the default value.
if xtype != "":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this line? I think empty string is not in XQUANTITIES?

Copy link
Contributor Author

@bobleesj bobleesj Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From our current init implementation, if xtype is not provided for DiffractionObjects, then xtype becomes an empty string as shown here:

class DiffractionObject:
    def __init__(
        self, name=None, wavelength=None, scat_quantity=None, metadata=None, xarray=None, yarray=None, xtype=None
    ):
        ...
        self.metadata = metadata
        if xtype is None:
            xtype = ""

Hence, we need to have if xtype != "" to ensure that we do not provide the warning msg when no value is provided for xtype, as you mentioned, "" is not in XQUANTITIES

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. I guess this is left over from our/my desire to allow people to instantiate an empty DO and then fill it later. Let's leave it like that, but we may be accumulating annoying technical debt that we wouldn't have if we went away from that choice.

if xtype not in XQUANTITIES:
raise ValueError(_xtype_wmsg(xtype))

def _get_original_array(self):
if self.input_xtype in QQUANTITIES:
if self._input_xtype in QQUANTITIES:
return self.on_q(), "q"
elif self.input_xtype in ANGLEQUANTITIES:
elif self._input_xtype in ANGLEQUANTITIES:
return self.on_tth(), "tth"
elif self.input_xtype in DQUANTITIES:
elif self._input_xtype in DQUANTITIES:
return self.on_d(), "d"

def on_q(self):
Expand All @@ -365,8 +392,8 @@ def on_d(self):
return [self.all_arrays[:, 3], self.all_arrays[:, 0]]

def scale_to(self, target_diff_object, xtype=None, xvalue=None):
f"""
returns a new diffraction object which is the current object but recaled in y to the target
"""
Return a new diffraction object which is the current object but recaled in y to the target

Parameters
----------
Expand Down Expand Up @@ -401,8 +428,8 @@ def scale_to(self, target_diff_object, xtype=None, xvalue=None):
return scaled

def on_xtype(self, xtype):
f"""
return a list of two 1D np array with x and y data, raise an error if the specified xtype is invalid
"""
Return a list of two 1D np array with x and y data, raise an error if the specified xtype is invalid

Parameters
----------
Expand Down
48 changes: 37 additions & 11 deletions tests/test_diffraction_objects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,16 +211,15 @@ def test_on_xtype():
assert np.allclose(test.on_xtype("d"), [np.array([12.13818, 6.28319]), np.array([1, 2])])


def test_on_xtype_bad():
test = DiffractionObject()
def test_init_invalid_xtype():
with pytest.raises(
ValueError,
match=re.escape(
f"I don't know how to handle the xtype, 'invalid'. Please rerun specifying an "
f"xtype from {*XQUANTITIES, }"
f"I don't know how to handle the xtype, 'invalid_type'. "
f"Please rerun specifying an xtype from {*XQUANTITIES, }"
),
):
test.on_xtype("invalid")
DiffractionObject(xtype="invalid_type")


params_index = [
Expand Down Expand Up @@ -287,7 +286,7 @@ def test_dump(tmp_path, mocker):
{
"_all_arrays": np.empty(shape=(0, 4)), # instantiate empty
"metadata": {},
"input_xtype": "",
"_input_xtype": "",
"name": "",
"scat_quantity": None,
"qmin": np.float64(np.inf),
Expand All @@ -304,7 +303,7 @@ def test_dump(tmp_path, mocker):
{
"_all_arrays": np.empty(shape=(0, 4)),
"metadata": {"thing": "1", "another": "2"},
"input_xtype": "",
"_input_xtype": "",
"name": "test",
"scat_quantity": "x-ray",
"qmin": np.float64(np.inf),
Expand Down Expand Up @@ -332,7 +331,7 @@ def test_dump(tmp_path, mocker):
]
),
"metadata": {},
"input_xtype": "tth",
"_input_xtype": "tth",
"name": "",
"scat_quantity": None,
"qmin": np.float64(0.0),
Expand Down Expand Up @@ -361,7 +360,7 @@ def test_dump(tmp_path, mocker):
]
),
"metadata": {},
"input_xtype": "d",
"_input_xtype": "d",
"name": "",
"scat_quantity": "x-ray",
"qmin": np.float64(0.0),
Expand Down Expand Up @@ -406,12 +405,39 @@ def test_all_array_setter():
# 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`.",
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))


def test_xarray_yarray_length_mismatch():
with pytest.raises(
ValueError,
match="'xarray' and 'yarray' must have the same length. "
"Please re-initialize 'DiffractionObject' or re-run the method 'insert_scattering_quantity' "
"with 'xarray' and 'yarray' of identical length",
):
DiffractionObject(xarray=np.array([1.0, 2.0]), yarray=np.array([0.0, 0.0, 0.0]))


def test_input_xtype_getter():
do = DiffractionObject(xtype="tth")
assert do.input_xtype == "tth"


def test_input_xtype_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.

Not allowed to reset _input_data

do = DiffractionObject(xtype="tth")

# Attempt to directly modify the property
with pytest.raises(
AttributeError,
match="Direct modification of attribute 'input_xtype' is not allowed. "
"Please use 'insert_scattering_quantity' to modify 'input_xtype'.",
):
do.input_xtype = "q"


def test_copy_object():
do = DiffractionObject(
name="test",
Expand Down
Loading