Refactor `add` operation in `DiffractionObject` and add tests #285

bobleesj · 2024-12-27T19:48:34Z

Starting with __add__ first, and will make separate PRs for subtraction, multiplication, division, etc. after review

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-27T19:49:28Z

src/diffpy/utils/diffraction_objects.py


-    def __radd__(self, other):


radd i think we don't need?

I think we may. Are you sure? Anyway, we can test and see.

you are correct - added __radd__ back using __radd__ = __add__

please see a new test below for do + scalar as well as scalar + do

codecov · 2024-12-27T19:49:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3f2d0a4) to head (da70bd6).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #285   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          408       446   +38     
=========================================
+ Hits           408       446   +38

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <100.00%> (ø)`
tests/test_diffraction_objects.py	`100.00% <100.00%> (ø)`

bobleesj · 2024-12-27T19:50:15Z

src/diffpy/utils/diffraction_objects.py

-            multiplied.on_q[1] = self.on_q[1] * other.on_q[1]
-        return multiplied
-
-    def __rmul__(self, other):


removed since identical as __mul__

We should test and make sure it is ok to remove this. On balance, I probably want to leave the 'r' functionalities

tests/test_diffraction_objects.py

bobleesj · 2024-12-27T19:51:10Z

tests/test_diffraction_objects.py

+
+
+@pytest.mark.parametrize(
+    "LHS_all_arrays, RHS_all_arrays, expected_all_arrays_sum",


test adding do1 + do2

bobleesj · 2024-12-27T19:51:35Z

tests/test_diffraction_objects.py

+
+
+@pytest.mark.parametrize(
+    "starting_all_arrays, scalar_value, expected_all_arrays",


test adding do + number

bobleesj · 2024-12-27T19:51:55Z

tests/test_diffraction_objects.py

+    # Add a string to a DO object, expect TypeError, only scalar (int, float) allowed for addition
+    do_LHS = do_minimal_tth
+    with pytest.raises(TypeError, match=re.escape(invalid_add_type_error_msg)):
+        do_LHS + "string_value"


string not allowed

bobleesj · 2024-12-27T19:52:03Z

tests/test_diffraction_objects.py

+    do_LHS = do_minimal
+    do_RHS = do_minimal_tth
+    with pytest.raises(ValueError, match=re.escape(x_grid_size_mismatch_error_msg)):
+        do_LHS + do_RHS


different lengths of xarray, not allowed

bobleesj

@sbillinge ready for review

sbillinge

Please see inline.

sbillinge · 2024-12-27T22:11:22Z

src/diffpy/utils/diffraction_objects.py


-    def __radd__(self, other):


I think we may. Are you sure? Anyway, we can test and see.

sbillinge · 2024-12-27T22:26:48Z

src/diffpy/utils/diffraction_objects.py

-            multiplied.on_q[1] = self.on_q[1] * other.on_q[1]
-        return multiplied
-
-    def __rmul__(self, other):


We should test and make sure it is ok to remove this. On balance, I probably want to leave the 'r' functionalities

tests/test_diffraction_objects.py

bobleesj · 2024-12-28T04:44:35Z

tests/test_diffraction_objects.py

+    do = do_minimal_tth
+    assert np.allclose(do.all_arrays, starting_all_arrays)
+    do_sum_RHS = do + scalar_to_add
+    do_sum_LHS = scalar_to_add + do


test for __radd__

bobleesj

@sbillinge

ready for review

right, radd is back. rmul is also back.

If this looks okay, then I will further implement the rest of the remaining operations

src/diffpy/utils/diffraction_objects.py

sbillinge

please see inline comments.

src/diffpy/utils/diffraction_objects.py

sbillinge · 2024-12-28T14:51:44Z

src/diffpy/utils/diffraction_objects.py

+invalid_add_type_emsg = (
+    "You may only add a DiffractionObject with another DiffractionObject or a scalar value. "
+    "Please rerun by adding another DiffractionObject instance or a scalar value. "
+    "e.g., my_do_1 + my_do_2 or my_do + 10"


for completeness, lets also add a radd as an example here. We could say "to add 10 to all intensities use..."

sbillinge · 2024-12-28T14:56:39Z

src/diffpy/utils/diffraction_objects.py

+
+        Examples
+        --------
+        Add a scalar value to the xarrays of the DiffractionObject instance:


let's add the radd example here too.

tests/test_diffraction_objects.py

sbillinge · 2024-12-28T18:06:53Z

tests/test_diffraction_objects.py

+@pytest.mark.parametrize(
+    "starting_all_arrays, scalar_to_add, expected_all_arrays",
+    [
+        # Test scalar addition to xarray values (q, tth, d) and expect no change to yarray values


the add operation is supposed to operate on the yarray leaving xarrays unaffected.

bobleesj · 2024-12-28T18:52:34Z

@sbillinge

the add operation is supposed to operate on the yarray leaving xarrays unaffected.

I see, I just refactored the earlier code which had xarrays adding up

        if isinstance(other, int) or isinstance(other, float) or isinstance(other, np.ndarray):
            summed.on_tth[1] = self.on_tth[1] + other
            summed.on_q[1] = self.on_q[1] + other

I will fix these.

sbillinge · 2024-12-28T21:38:47Z

@sbillinge

the add operation is supposed to operate on the yarray leaving xarrays unaffected.

I see, I just refactored the earlier code which had xarrays adding up
        if isinstance(other, int) or isinstance(other, float) or isinstance(other, np.ndarray):
            summed.on_tth[1] = self.on_tth[1] + other
            summed.on_q[1] = self.on_q[1] + other
I will fix these.

Those were the y-arrays 😂. On_q[0] is the q and on_q[1] the intensities so it is more logical for users when plotting.

…-ops

bobleesj · 2024-12-29T05:57:05Z

src/diffpy/utils/diffraction_objects.py

+        >>> new_do = my_do_1 + my_do_2
+        """
+
+        self._check_operation_compatibility(other)


Created a private func that checks the validity of other

def _check_operation_compatibility(self, other): if not isinstance(other, (DiffractionObject, int, float)): raise TypeError(invalid_add_type_emsg) if isinstance(other, DiffractionObject): self_yarray = self.all_arrays[:, 0] other_yarray = other.all_arrays[:, 0] if len(self_yarray) != len(other_yarray): raise ValueError(y_grid_length_mismatch_emsg)

Nice! it may be more readable if we use

if shape(self.all_arrays) != shape(other.all_arrays)

to accomplish the same thing?

src/diffpy/utils/diffraction_objects.py

bobleesj · 2024-12-29T06:00:39Z

@sbillinge ready for review! - addressed all of the comments

sbillinge

I will merge this, but please see my comment on the private function. No need to make a special PR for that, but if you want, fix i tin any other PR....... or not.....

sbillinge · 2024-12-29T12:48:01Z

src/diffpy/utils/diffraction_objects.py

+        >>> new_do = my_do_1 + my_do_2
+        """
+
+        self._check_operation_compatibility(other)


Nice! it may be more readable if we use

if shape(self.all_arrays) != shape(other.all_arrays)

to accomplish the same thing?

bobleesj · 2024-12-29T13:00:52Z

I will merge this, but please see my comment on the private function. No need to make a special PR for that, but if you want, fix i tin any other PR....... or not.....

Yup will do 👍

bobleesj added 5 commits December 27, 2024 14:40

Refactor __eq__ function for DO

7db3a4f

Add __eq__ news and test

3d4841b

Add example to __eq__

5d0ebcc

Revert not needed change to naming in a test func

9741a8e

Apply pre-commit

3f577d7

bobleesj commented Dec 27, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

bobleesj commented Dec 27, 2024

View reviewed changes

tests/test_diffraction_objects.py Show resolved Hide resolved

bobleesj commented Dec 27, 2024

View reviewed changes

bobleesj changed the title ~~Fix __add__ operation in DiffractionObject and add tests~~ Refactor __add__ operation in DiffractionObject and add tests Dec 27, 2024

bobleesj mentioned this pull request Dec 27, 2024

write tests for... #187

Closed

9 tasks

sbillinge reviewed Dec 27, 2024

View reviewed changes

bobleesj and others added 2 commits December 27, 2024 23:42

Add radd and rmul back and add tests

d561583

[pre-commit.ci] auto fixes from pre-commit hooks

ac5a2f3

bobleesj commented Dec 28, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

sbillinge reviewed Dec 28, 2024

View reviewed changes

bobleesj added 3 commits December 28, 2024 22:54

Merge branch 'main' of https://github.com/diffpy/diffpy.utils into do…

12848e8

…-ops

refactor: add yarrays together instead of xarrays for __add__

11c4166

test: combine do with another do or scalar value tests

846c72a

bobleesj commented Dec 29, 2024

View reviewed changes

src/diffpy/utils/diffraction_objects.py Show resolved Hide resolved

test: changed xarray to yarray in one of the test cases

da70bd6

sbillinge reviewed Dec 29, 2024

View reviewed changes

sbillinge merged commit 1ea8e9a into diffpy:main Dec 29, 2024
5 checks passed

bobleesj deleted the do-ops branch December 29, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `add` operation in `DiffractionObject` and add tests #285

Refactor `add` operation in `DiffractionObject` and add tests #285

bobleesj commented Dec 27, 2024 •

edited

Loading

bobleesj Dec 27, 2024

sbillinge Dec 27, 2024

bobleesj Dec 28, 2024

codecov bot commented Dec 27, 2024 •

edited

Loading

bobleesj Dec 27, 2024

sbillinge Dec 27, 2024

bobleesj Dec 27, 2024

bobleesj Dec 27, 2024

bobleesj Dec 27, 2024

bobleesj Dec 27, 2024

bobleesj left a comment

sbillinge left a comment

sbillinge Dec 27, 2024

sbillinge Dec 27, 2024

bobleesj Dec 28, 2024

bobleesj left a comment •

edited

Loading

sbillinge left a comment

sbillinge Dec 28, 2024

sbillinge Dec 28, 2024

sbillinge Dec 28, 2024

bobleesj commented Dec 28, 2024

sbillinge commented Dec 28, 2024

bobleesj Dec 29, 2024

sbillinge Dec 29, 2024

bobleesj commented Dec 29, 2024 •

edited

Loading

sbillinge left a comment

sbillinge Dec 29, 2024

bobleesj commented Dec 29, 2024



		@pytest.mark.parametrize(
		"LHS_all_arrays, RHS_all_arrays, expected_all_arrays_sum",



		@pytest.mark.parametrize(
		"starting_all_arrays, scalar_value, expected_all_arrays",

Refactor __add__ operation in DiffractionObject and add tests #285

Refactor __add__ operation in DiffractionObject and add tests #285

Conversation

bobleesj commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj left a comment • edited Loading

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 28, 2024

sbillinge commented Dec 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 29, 2024 • edited Loading

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 29, 2024

Refactor `add` operation in `DiffractionObject` and add tests #285

Refactor `add` operation in `DiffractionObject` and add tests #285

bobleesj commented Dec 27, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading

bobleesj left a comment •

edited

Loading

bobleesj commented Dec 29, 2024 •

edited

Loading