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
sbillinge 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 3 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>
48 changes: 37 additions & 11 deletions src/diffpy/utils/diffraction_objects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ def _xtype_wmsg(xtype):
)


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


class DiffractionObject:
def __init__(
self, name=None, wavelength=None, scat_quantity=None, metadata=None, xarray=None, yarray=None, xtype=None
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

@sbillinge sbillinge Dec 9, 2024

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

@bobleesj bobleesj Dec 12, 2024

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

@sbillinge sbillinge Dec 12, 2024

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

@sbillinge sbillinge Dec 12, 2024

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 xtype(self):
return self._xtype

@xtype.setter
def xtype(self, _):
raise AttributeError(_setter_wmsg("xtype"))
Copy link
Contributor Author

@bobleesj bobleesj Dec 8, 2024

Choose a reason for hiding this comment

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

@sbillinge Like Would you also like to prevent the user from directly modifying 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._xtype
Copy link
Contributor Author

@bobleesj bobleesj Dec 8, 2024

Choose a reason for hiding this comment

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

If we do not want the user from directly modifying xtype, like _all_arrays, _xtype is used internally.

Copy link
Contributor

@yucongalicechen yucongalicechen Dec 8, 2024

Choose a reason for hiding this comment

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

I like how you changed input_xtype to xtype! I do think people can directly modify xtype though (@sbillinge please confirm). For example I might want to plot my diffraction object on tth first, and then change its xtype to q so I can visualize it in qspace.

Copy link
Contributor

@sbillinge sbillinge Dec 8, 2024

Choose a reason for hiding this comment

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

@yucongalicechen that is not how it works. the attribute was called input_xtype because we wanted to keep a record of the xtype input by the user. So I don't want to change its name. When xtype is used for plotting any xtype can be given, for example diffobj.on_xtype('q') would satisfy @yucongalicechen 's UC even if the data were input on a tth grid.

Since we only use it internally, we could change the attribute name to _input_xtype to make it private. I think it should only be set by insert_data and not re-settable.

Copy link
Contributor Author

@bobleesj bobleesj Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@sbillinge - done. input_type is used internally and it st only gettable.

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 @@ -335,7 +348,7 @@ def insert_scattering_quantity(
"""
self._set_xarrays(xarray, xtype)
self._all_arrays[:, 0] = yarray
self.input_xtype = xtype
self._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 +360,25 @@ def insert_scattering_quantity(
if wavelength is not None:
self.wavelength = wavelength

# 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."
)

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

@sbillinge sbillinge Dec 9, 2024

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

@sbillinge sbillinge Dec 12, 2024

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._xtype in QQUANTITIES:
return self.on_q(), "q"
elif self.input_xtype in ANGLEQUANTITIES:
elif self._xtype in ANGLEQUANTITIES:
return self.on_tth(), "tth"
elif self.input_xtype in DQUANTITIES:
elif self._xtype in DQUANTITIES:
return self.on_d(), "d"

def on_q(self):
Expand Down
34 changes: 25 additions & 9 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"I don't know how to handle the xtype, 'invalid_type'. Please rerun specifying an "
f"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": "",
"_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": "",
"_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",
"_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",
"_xtype": "d",
"name": "",
"scat_quantity": "x-ray",
"qmin": np.float64(0.0),
Expand Down Expand Up @@ -407,6 +406,23 @@ def test_all_array_setter():
with pytest.raises(
AttributeError,
match="Direct modification of attribute 'all_arrays' is not allowed."
"Please use 'insert_scattering_quantity' to modify `all_arrays`.",
"Please use 'insert_scattering_quantity' to modify 'all_arrays'.",
):
actual_do.all_arrays = np.empty((4, 4))


def test_xtype_getter():
do = DiffractionObject(xtype="tth")
assert do.xtype == "tth"
Copy link
Contributor Author

@bobleesj bobleesj Dec 8, 2024

Choose a reason for hiding this comment

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

Test getter for xtype



def test_xtype_setter():
Copy link
Contributor Author

@bobleesj bobleesj Dec 8, 2024

Choose a reason for hiding this comment

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

Test setter for xtype - not allowed

do = DiffractionObject(xtype="tth")

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