[SyneRBI/SIRF] Updates for mixed data algebra based on __array_priority__. (PR #1373)
Evgueni Ovtchinnikov
Changes in this pull request
Replaces quick fix in #1357 with a proper fix employing __array_priority__ feature and adds more mixed algebra tests.
Testing performed
Related issues
Checklist before requesting a review
- I have performed a self-review of my code
- I have added docstrings/doxygen in line with the guidance in the developer guide
- I have implemented unit tests that cover any new or modified functionality
- The code builds and runs on my machine
- CHANGES.md has been updated with any functionality change
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following:
- The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF/pull/1373
Commit Summary
- d95c7bb updated files for release 3.9.0 [ci skip]
- 54505e7 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- dedf83c Merged branch 'master' of https://github.com/SyneRBI/SIRF
- a164114 added numpy array to possible SIRF image data algebra operands
- c53d80b added to User Guide a subsection on SIRF/numpy data algebra peculiarities [ci skip]
- 9f2ecf8 updated CHANGES.md [ci skip]
- 80b3c07 added tests for #1358
- 57d9e15 attended to Codacy issues
- 0477cc0 simpliied mixed data algebra
- 17867e4 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- f249547 resolved conflicts
- 5a1e82f Merge branch 'master' of https://github.com/SyneRBI/SIRF
- 6bfdf52 fixed mixing SIRF and numpy data algebra issue #1357
- 9b67e9e added algebra tests for ax/x, 2/x etc.
- a00488d Merge branch 'master' of https://github.com/SyneRBI/SIRF
- c3aa576 Merge branch 'master' into mda-priority-fix
File Changes
(2 files)
- M src/common/SIRF.py (11)
- M src/common/Utilities.py (35)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
As I don't know anything about __array_priority__, I'll leave it to @paskino and @casperdcl to review.
BTW, what happens if the sizes are wrong? Shouldn't we add some check?
This will have to be squash merged (after modifying CHANGES and the user's guide appropriately), due to the somewhat horrible history.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
are we sure we really want to use as_array() for these tests? Obviously, if replacing it with asarray() we'd have to be very careful for any operatoins that modify the object.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
better safe than sorry IMHO
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
BTW, what happens if the sizes are wrong?
assert_validities would raise AssertionError
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
better safe than sorry IMHO
not necessarily for tests. Probably a good idea to do at least some of the tests with asarray() as well, as the future, it's what is must likely to hit us.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov commented on this pull request.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
Actually, replacing every as_array use with that of asarray is perfectly safe as asarray defaults to copy - to get a view, you need asarray(copy=False) (we probably should keep as_array method in our data container classes for backward compatibility). However, this needs a separate PR I believe, this one just fixes #1357.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 856c7ee added the result type checks for mixed data algebra
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 0d8f93f [ci skip] updated User Guide description of data algebra and CHANGES.md
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
@KrisThielemans commented on this pull request.
> @@ -790,24 +790,47 @@ def data_container_algebra_tests(test, x, eps=1e-4):
t = numpy.linalg.norm(az)
test.check_if_zero_within_tolerance(s, eps * t)
+ z = ay*x
+ az = z.as_array()
asarraydefaults to copy
no it doesn't. it defaults to view (if the container supports it).
Whatever, I still feel we need to test with the most used function, even if you know in the end it gives the same results (but at some point, it might not)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.