Remove rarely used shape utilities #1016
Conversation
ricardoV94
force-pushed
the
cleanup_shape_feat
branch
from
0099ecc to
59ff115
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1016 +/- ##
==========================================
- Coverage 81.75% 81.73% -0.03%
==========================================
Files 183 183
Lines 47756 47734 -22
Branches 11620 11611 -9
==========================================
- Hits 39044 39016 -28
- Misses 6519 6525 +6
Partials 2193 2193
|
| if axis is not None and axis < 0: | ||
| raise ValueError("Axis cannot be negative.") |
There was a problem hiding this comment.
Because it simplifies the logic in the Op. The helper users use pt.unique handles the negative axis and passes a positive one to the Op. Users don't really create Ops themselves
| ret[0] = tuple( | ||
| fgraph.shape_feature.shape_ir(i, node.outputs[0]) for i in range(ndim) | ||
| ) | ||
| [x_shape] = i0_shapes |
There was a problem hiding this comment.
I am not sure I understand what is happening in this function, but just to check, shouldn't there be a case for return_index and return_counts as well?
There was a problem hiding this comment.
There is. i0_shapes are the input dimensions, so that doesn't change with the number of outputs. return_index/counts are outputs, and they are always vector.
We set out_shapes = [out.shape[0] for out in node.outputs] by default which will always work for return_index and return_counts. Then we have special logic for the main output when axis is not None and for return_inverse which is not just out.shape[0].
There was a problem hiding this comment.
The big picture is the function tries to return a graph for the shape of the outputs given the input shapes (and possibly values, which you could retrieve from node.inputs). The default graph of the shape is just output.shape, which we try to avoid when possible, as we would like to avoid computing the Op in order to find out its shape.
For unique we can do that for some of the outputs dimensions, but not all (we only know how many repeated values there are if we evaluate Unique).
This method is combining dims we can know from the input shapes and those that we can only get after we compute the outputs with out.shape[0] or out.shape[x].
| @@ -1293,6 +1269,9 @@ def unique( | |||
| * the number of times each unique value comes up in the input array | |||
|
|
|||
| """ | |||
| ar = as_tensor_variable(ar) | |||
| if axis is not None: | |||
| axis = normalize_axis_index(axis, ar.ndim) | |||
There was a problem hiding this comment.
@Armavica here is where we allow negative axis for the user
This PR cleans up the use of obscure shape utilities, removing them from Unique.infer_shape and and other places where they are not needed.