Updates for mixed data algebra based on __array_priority__.#1373
Updates for mixed data algebra based on __array_priority__.#1373evgueni-ovtchinnikov merged 18 commits intomasterfrom
Conversation
KrisThielemans
left a comment
KrisThielemans
left a comment
There was a problem hiding this comment.
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.
| test.check_if_zero_within_tolerance(s, eps * t) | ||
|
|
||
| z = ay*x | ||
| az = z.as_array() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
better safe than sorry IMHO
There was a problem hiding this comment.
BTW, what happens if the sizes are wrong?
assert_validities would raise AssertionError
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
deleting as_array will break compatibility with CIL.
4 participants
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
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following: