BUG: fix in-out aliasing issue in OperatorSum, closes #1181 #1225
Conversation
|
I guess I should add a test. |
kohr-h
force-pushed
the
issue-1181__op_aliasing_bug
branch
from
5951a30 to
876b8c2
Compare
|
Operator tests adapted for that, and they surfaced the same issue with |
|
Numpy <1.13 apparently has an issue with |
kohr-h
force-pushed
the
issue-1181__op_aliasing_bug
branch
from
876b8c2 to
7cd1c4b
Compare
| self.matrix.dot(x, out=out_arr) | ||
| if (parse_version(np.__version__) < parse_version('1.13.0') and | ||
| x is out): | ||
| # Workaround for bug in Numpy < 1.13 with aliased in and |
There was a problem hiding this comment.
wait, do we even allow np>=1.13 atm? or is this future profing?
There was a problem hiding this comment.
No, not currently. But since the last conda update (an ASTRA update) it has become notoriously hard to install a lower version, also since they forgot to add Numpy 1.12 with nomkl.
So it's easier than ever to install the wrong version 1.13 of Numpy. That will break some of the tests, but at least there are no extra bugs. And the aliased version gives a speedup that would be nice to have when not broken.
There was a problem hiding this comment.
Ok, how much is left until we have tensors in and can handle this "natively" so to say?
There was a problem hiding this comment.
There's one bug with the Huber functional that breaks the large-scale test, and it's not on master. Need to check. Then there is the breakage of the tensorflow contrib code which I imagine is not unimportant :-). You know what to change there I guess, I only did the obvious stuff (size -> shape roughly). Then, I'll check again the examples. Other than that, it's ready I think.
There was a problem hiding this comment.
Well getting this fixed before the course would be great so we have better TF interop for then, I suggest we try to rush that and get it somewhat battle-tested before the 12:th next month
| with writable_array(out) as out_arr: | ||
| self.matrix.dot(x, out=out_arr) | ||
| if (parse_version(np.__version__) < parse_version('1.13.0') and | ||
| x is out): |
There was a problem hiding this comment.
This does not catch all cases, e.g. we could have to containers that are possibly alliased, overall this is a rather hard problem to solve, see e.g.
https://docs.scipy.org/doc/numpy-dev/reference/generated/numpy.shares_memory.html
https://docs.scipy.org/doc/numpy-dev/reference/generated/numpy.may_share_memory.html
but I guess this solves some of the cases?
There was a problem hiding this comment.
True, this isn't a perfect fix, but for our current classes it should handle everything. In particular since we don't support MatrixOperator on product spaces (where things can become hairy).
| assert all_almost_equal(out, expected) | ||
| def test_operator_call(dom_eq_ran): | ||
| """Check operator evaluation against NumPy reference.""" | ||
| if dom_eq_ran: |
There was a problem hiding this comment.
prefer adding a "shape" fixture?
There was a problem hiding this comment.
That would make it quite complicated, e.g., when testing op1 + op2 shapes must be the same, when testing op1 * op2 you have another requirement, etc. This fixture is really only intended to generate a test case where aliased input and output are checked.
There was a problem hiding this comment.
I can add a comment in the test.
|
I'll rebase, and unless the concerns with the fixture are significant I'll merge the fix. |
kohr-h
force-pushed
the
issue-1181__op_aliasing_bug
branch
from
7cd1c4b to
9b5e1b9
Compare
No description provided.