460 element equality #462
Open
460 element equality #462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 71.84% 71.94% +0.09%
==========================================
Files 46 46
Lines 6744 6768 +24
Branches 1496 1506 +10
==========================================
+ Hits 4845 4869 +24
Misses 1333 1333
Partials 566 566
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi @precicely, thanks for the PR. It looks great. Before I review the PR, it would be great if you could extend the test coverage for the new functionality. Thanks, |
|
No problem @waltsims. I've tweaked the code a bit and improved the coverage. |
waltsims
requested changes
Sep 3, 2024
Comment on lines
+122
to
+142
| def __eq__(self, other): | ||
| """Equality operator that handles all fields but specifically numpy | ||
| arrays. | ||
|
|
||
| :param other: an instance of Element | ||
| :raises ValueError: when other object is not an instance of Element | ||
| :return: bool | ||
| """ | ||
| if not isinstance(other, type(self)): | ||
| raise TypeError(f"{other} with {type(other)} is not of type Element") | ||
|
|
||
| for field in fields(self): | ||
| self_attr = getattr(self, field.name) | ||
| other_attr = getattr(other, field.name) | ||
| if isinstance(self_attr, np.ndarray): | ||
| if not np.array_equal(self_attr, other_attr): | ||
| return False | ||
| else: | ||
| if self_attr != other_attr: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
please move eq next to post_init to keep the dunder methods together.
Comment on lines
+14
to
+35
| def test_element(): | ||
| element1 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | ||
| # element2 has a very similar position to element 1 but with a small numerical difference | ||
| element2 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1.000001]) | ||
| # element3 is the same as element 1 but belongs to a differnet group | ||
| element3 = Element(group_id=1, type="rect", dim=2, active=True, measure=2, position=[1, 1, 1]) | ||
| # element4 is the same as element 1 but has a differnet position | ||
| element4 = Element(group_id=0, type="rect", dim=2, active=True, measure=2, position=[2, 2, 2]) | ||
| # element 4 is not an element | ||
| element5 = "not an element" | ||
|
|
||
| assert element1 == element1 | ||
| assert element1 != element2 | ||
| assert element1.is_close(element2) | ||
| assert element1 != element3 | ||
| assert not element1.is_close(element3) | ||
| assert element1 != element4 | ||
| assert not element1.is_close(element4) | ||
| with pytest.raises(TypeError): | ||
| element1 != element5 | ||
| with pytest.raises(TypeError): | ||
| element1.is_close(element5) |
There was a problem hiding this comment.
Minor nitpick and not blocking: It would be great for clarity if you breakout the tests into their own functions. Here is some inspiration:
Suggested change
| def test_element(): | |
| element1 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| # element2 has a very similar position to element 1 but with a small numerical difference | |
| element2 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1.000001]) | |
| # element3 is the same as element 1 but belongs to a differnet group | |
| element3 = Element(group_id=1, type="rect", dim=2, active=True, measure=2, position=[1, 1, 1]) | |
| # element4 is the same as element 1 but has a differnet position | |
| element4 = Element(group_id=0, type="rect", dim=2, active=True, measure=2, position=[2, 2, 2]) | |
| # element 4 is not an element | |
| element5 = "not an element" | |
| assert element1 == element1 | |
| assert element1 != element2 | |
| assert element1.is_close(element2) | |
| assert element1 != element3 | |
| assert not element1.is_close(element3) | |
| assert element1 != element4 | |
| assert not element1.is_close(element4) | |
| with pytest.raises(TypeError): | |
| element1 != element5 | |
| with pytest.raises(TypeError): | |
| element1.is_close(element5) | |
| def test_element_is_close(): | |
| base_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| # Test cases | |
| assert base_element.is_close(Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1])) | |
| assert base_element.is_close(Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1.000001])) | |
| assert not base_element.is_close(Element(group_id=1, type="rect", dim=2, active=True, measure=2, position=[1, 1, 1])) | |
| assert not base_element.is_close(Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[2, 2, 2])) | |
| assert not base_element.is_close(Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, float('nan')])) | |
| assert not base_element.is_close(Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, float('inf')])) | |
| assert not base_element.is_close(Element(group_id=0, type="annular", dim=2, active=True, measure=1, position=[1, 1, 1])) | |
| def test_element_is_close_nan(): | |
| nan_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, float('nan')]) | |
| assert nan_element.is_close(nan_element, equal_nan=True) | |
| assert not nan_element.is_close(nan_element, equal_nan=False) | |
| def test_element_is_close_boundary(): | |
| base_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| boundary_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1 + 1.1e-5]) | |
| assert not base_element.is_close(boundary_element) | |
| assert base_element.is_close(boundary_element, rtol=1.2e-5) | |
| def test_element_is_close_consistency(): | |
| base_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| other_element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1.000001]) | |
| assert base_element.is_close(other_element) == np.allclose(base_element.position, other_element.position) | |
| assert base_element.is_close(other_element) == base_element.is_close(other_element, rtol=1e-05, atol=1e-08, equal_nan=False) | |
| def test_element_is_close_type_error(): | |
| element = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| with pytest.raises(TypeError, match=r"not an element with <class 'str'> is not of type Element"): | |
| element.is_close("not an element") | |
| def test_element_equality(): | |
| element1 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1]) | |
| element2 = Element(group_id=0, type="rect", dim=2, active=True, measure=1, position=[1, 1, 1.000001]) | |
| element3 = Element(group_id=1, type="rect", dim=2, active=True, measure=2, position=[1, 1, 1]) | |
| assert element1 == element1 | |
| assert element1 != element2 | |
| assert element1 != element3 | |
Comment on lines
+94
to
+96
| """Compares 2 Elements to a certain precision for their numerical | ||
| fields. This differs from the __eq__ method which requires a perfect | ||
| match between numerical fields. Works with ints, floats and numpy |
There was a problem hiding this comment.
Please specify behavior for non-numerical fields as well in docstring. Namely, is_close allows numerical values to vary within the rtol but only when non-numerical fields are the same. When is_close returns False, it could be due to variations in numerical fields beyond rtol, OR because other properties differ.
There was a problem hiding this comment.
is_approximately_equal might be a more accurate name.
There was a problem hiding this comment.
Will do. The thinking with is_close was that it mirrors numpy's allclose and is_close but happy to change it to is_approximately_equal.
There was a problem hiding this comment.
Please move Element tests to a new file directly under tests. This directory is for the comparison of Python outputs and collected outputs from MATLAB.
thanks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Elementnow has__eq__method that works with numpy array as well ais_closemethod that useful for approximate equality testing.