- Notifications
You must be signed in to change notification settings - Fork 21
feat: Support *, /, - operations between two DiffractionObjects or scalar #293
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| self_yarray = self.all_arrays[:, 0] | ||
| other_yarray = other.all_arrays[:, 0] | ||
| if len(self_yarray) != len(other_yarray): | ||
| if self_yarray.shape != other_yarray.shape: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge as suggested - using shape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea for using shape was to shorten code and make it more readable, so
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) is replaced by
if shape(self.all_arrays) != shape(other.all_arrays): raise ValueError(y_grid_length_mismatch_emsg) codecovbot commented Dec 29, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #293 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 8 8 Lines 446 483 +37 ========================================= + Hits 446 483 +37
|
| @pytest.mark.parametrize( | ||
| "starting_all_arrays, scalar_to_add, expected_all_arrays", | ||
| "operation, starting_yarray, scalar_value, expected_yarray", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operation b/w scalar and do
| (np.array([[1.0, 6.28318531, 100.70777771, 1], [2.0, 3.14159265, 45.28748053, 2.0]]),), | ||
| (np.array([[2.0, 0.51763809, 30.0, 12.13818192], [4.0, 1.0, 60.0, 6.28318531]]),), | ||
| (np.array([[2.0, 6.28318531, 100.70777771, 1], [4.0, 3.14159265, 45.28748053, 2.0]]),), | ||
| # Test addition, subtraction, multiplication, and division of two DO objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operation b/w do1 and do2
| def test_operator_invalid_type(do_minimal_tth, invalid_add_type_error_msg): | ||
| # Add a string to a DiffractionObject, expect TypeError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid case - add string
| @pytest.mark.parametrize("operation", ["add", "sub", "mul", "div"]) | ||
| def test_operator_invalid_yarray_length(operation, do_minimal, do_minimal_tth, y_grid_size_mismatch_error_msg): | ||
| # Add two DO objects with different yarray lengths, expect ValueError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid case - add different lengths of yarray between 2 DOs
bobleesj commented Dec 29, 2024
@sbillinge ready for review! |
yucongalicechen commented Dec 30, 2024
Do we want to add both metadata to the resulting diffraction object? |
sbillinge left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I tried to shorten the all_arrays shape check part of the code before merging
| self_yarray = self.all_arrays[:, 0] | ||
| other_yarray = other.all_arrays[:, 0] | ||
| if len(self_yarray) != len(other_yarray): | ||
| if self_yarray.shape != other_yarray.shape: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea for using shape was to shorten code and make it more readable, so
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) is replaced by
if shape(self.all_arrays) != shape(other.all_arrays): raise ValueError(y_grid_length_mismatch_emsg) 90fd625 into diffpy:mainUh oh!
There was an error while loading. Please reload this page.
bobleesj commented Dec 30, 2024
@sbillinge ah yes. Great! Thanks |
Closes#187 -
No docstrings made at the moment, but I will do in the next PR working on the issue #239