Remove transfer_type and default inplace Elemwise Ops
#1752
Conversation
ricardoV94
changed the title
transfer_type and stop defining inplace Elemwise Ops ahead of timetransfer_type and stop creating inplace default Elemwise Ops
ricardoV94
changed the title
transfer_type and stop creating inplace default Elemwise Opstransfer_type and default inplace Elemwise Ops
| install-xarray: [0] | ||
| part: | ||
| - "tests --ignore=tests/tensor --ignore=tests/scan --ignore=tests/xtensor" | ||
| - "tests --ignore=tests/scan --ignore=tests/tensor --ignore=tests/xtensor" |
There was a problem hiding this comment.
This removes the tensor/test_inplace.py and reorders the ignore / parts to be more readible. They ignore show up in the same order where each part is that reintroduced.
May still change after the PR runs to rebalance the workload so they take more or less the same time
There was a problem hiding this comment.
Jobs look well balanced
| complex2=(random_complex(2, 3, rng=rng), random(2, 3, rng=rng)), | ||
| empty=(np.asarray([], dtype=config.floatX), np.asarray([1], dtype=config.floatX)), | ||
| ) | ||
| TestAddInplaceBroadcast = makeBroadcastTester( |
There was a problem hiding this comment.
Kept one just for good measure
ricardoV94
force-pushed
the
transfer_type
branch
from
b3f5b06 to
b1d0704
Compare
| Zt = Z.transpose() | ||
| assert isinstance(Zt.owner.op, DimShuffle) and Zt.owner.op.view_map == {0: [0]} | ||
| with pytest.raises(InconsistencyError, match=Gemm.E_z_uniq): | ||
| gemm_inplace(Z, 1.0, A, inplace.transpose_inplace(Z), 1.0) | ||
| gemm_inplace(Z, 1.0, A, Zt, 1.0) |
There was a problem hiding this comment.
In the very old times DimShuffle wasn't a view by default, only after compilation. This transpose_inplace would create one that was a view by default. Now that it is always, it wasn't needed. I added an explicit assert
|
Just some minor failure, ready for review |
ricardoV94
force-pushed
the
transfer_type
branch
2 times, most recently
from
63f2dab to
7fd9ea4
Compare
| try: | ||
| return type(op)(scalar_op, inplace_pattern).make_node(*node.inputs) | ||
| except TypeError: | ||
| # Elemwise raises TypeError if we try to inplace an output on an input of a different dtype |
There was a problem hiding this comment.
No prior test caused this to happen, and I suspect it would be rare, but added an explicit handling and test.
In general it would be nice that node.op.make_node(*node.inputs) would always return the same type, but global config variables can change the behavior of Scalar.make_node. The transfer_type was working as a kind of freeze_output_type here I guess.
We can revisit this approach later, and perhaps put the override behavior at the level of Elemwise, not ScalarOp. But for now I'm inclined to go with simplicity.
Note that with the default config.cast_policy="custom" or optional config.cast_policy="numpy" floatX plays no role in Scalar.make_node, it only affects the initial creation of tensor/constants. Only config.cast_policy="numpy+floatX" could lead to this edge case.
There was a problem hiding this comment.
PS: This sort of issue could show up also during rewrites, since it is invalid to return a replacement of a different dtype. So even something like log(add(1, x)) -> log1p(x) could be problematic, and always requires an explicit cast for safety. This just shows how the behavior puts complexity everywhere, and this was but one instance of that.
ricardoV94
force-pushed
the
transfer_type
branch
from
7fd9ea4 to
ef7838a
Compare
…acing This helper could arbitrarily override the default output_type from `ScalarOp.make_node` so that the output type matched one of the input types. This can be used to create artificial Op signatures that don't make sense or can't be cleanly implemented in other backends. For instance an Add with signature (int8,int64)->int8. This helper was historically used in: 1. Elemwise inplace rewrite. I assume as a preventive measure. However, regular use should never require changing the ScalarOp signature, as we only try to inplace on inputs that match the output dtype and recreating the same Op with the same input types should always return the same output type. ScalarOp don't have a concept of inplace, only the Elemwise wrapper does, and it shouldn't require recreating/mutating the original Op. 2. SecondOp. Here it makes sense, but a custom static_method works just as well 3. Inplace tests with the inplace variants of `@scalar_elemwise` decorator. These test Classes were removed. It still didn't make sense to test/force Ops to have an artifical signature for the sake of tests. They were removed anyway
ricardoV94
force-pushed
the
transfer_type
branch
from
ef7838a to
d882c8a
Compare
2 participants
This is a spinoff of #811
Background
Some inplace tests started failing when we switched to numba backend by default. These tests were defining Elemwise/ ScalarOp variants that didn't make much sense.
The decorator
@scalar_elemwisewhen called on a function withinplacesuffix would create a inplace version of Elemwise that would write the first output on the first input. This isn't always safe as we can only do that when the dtypes match. To work around this the ScalarOps were being re-created withtransfer_typewhich would override regularmake_nodelogic to force the output type to match one of the first input type, even if this doesn't make sense likeadd(int8, int64)->(int8).These are the sort of cases that failed in numba backend, in part because of missing #1747, in part because it would require a lot of logic on custom ScalarOp implementation (those that don't fallback to either cython or python obj mode), because we would always have to go back and check if the output dtype matched the promised one from the ScalarOp, even when it makes no sense.
Changes
@scalar_elemwiseand default inplace Elemwise Opstransfer_typewhich was mostly used for those dummy Op/tests and