Lower cummin op #8565
Lower cummin op #8565
Conversation
| @@ -289,6 +288,7 @@ def get_allowed_ops_map( | |||
| # AllowedOpInfoEntry('cosh'), | |||
| # AllowedOpInfoEntry('cov'), | |||
| # AllowedOpInfoEntry('cummax'), | |||
| # AllowedOpInfoEntry('cummin'), | |||
There was a problem hiding this comment.
@zyy-martin Thanks for your contributing.
Could you add a test for the op execution result's correctness?
There was a problem hiding this comment.
On this same note, could you add a test where dim is a 0-sized dimension?
Since this is using the same lowering as cummax, it could also have the same problems. ref: #8610
There was a problem hiding this comment.
It has the same problem as cummax, as well as old ops such as cumprod and cumsum. I assume that was the reason those tests were excluded in the first place.
We can consider modify the op tests to have the option to exclude scalars/0-dimension values to cover other test cases. The C++ tests in this PR have verified the correctness though.
There was a problem hiding this comment.
Right now the SampleInput includes scalers by default here https://github.com/pytorch/pytorch/blob/dddf52b1b91d473e249829dad6c705c13624b35f/torch/testing/_internal/common_methods_invocations.py#L6942. I believe we can override the sample function in our test_ops.py to exclude the zero-dimension test cases. As for why the current implementation does not support scalar, i think we will need more investigation.
There was a problem hiding this comment.
Although the test suite only support 0-sized dim, we can write simple tests from scratch to compare the result of torch/xla with native pytorch on cpu.
There was a problem hiding this comment.
I agree with @yaochengji here to include a test for result correctness.
Otherwise, LGTM.
| @@ -289,6 +288,7 @@ def get_allowed_ops_map( | |||
| # AllowedOpInfoEntry('cosh'), | |||
| # AllowedOpInfoEntry('cov'), | |||
| # AllowedOpInfoEntry('cummax'), | |||
| # AllowedOpInfoEntry('cummin'), | |||
There was a problem hiding this comment.
Moving the discussion about 0-sized dimensions here, so as not to flood the other thread.
Do you think it would be hard to add support to 0-sized dimensions?
I'm asking for that, because I think this is a BC-breaking change. Before your PR, cummin would fallback to CUDA, which would work with 0-sized dimensions. Now, it crashes (#8610).
That said, since other operations (e.g. cummax) also have this problem, I think we could merge this and work out a solution to this issue afterwards.
There was a problem hiding this comment.
Do you think it would be hard to add support to 0-sized dimensions?
I am not sure, the error suggests it might have come from the XLA backend: https://github.com/openxla/xla/blob/e795171be9897e1356b1d98588e4fe784e2fc1bb/xla/service/shape_inference.cc#L202. will need more time to figure out a fix.
That said, since other operations (e.g.
cummax) also have this problem, I think we could merge this and work out a solution to this issue afterwards.
Agree. It also applies to core ops like cumprod/cumsum which were lowered since the beginning of ptxla.
There was a problem hiding this comment.
Since the XLA backend doesn't work with 0-sized dimensions, maybe we could catch this while tracing (since the shapes should be static). Then, return 2 0-sized tensors.
That said, I believe this could be left for a future PR (maybe also solving the other ops that have the same issue).
There was a problem hiding this comment.
Thanks. Is there anything else you'd want me to add in this PR?
There was a problem hiding this comment.
I also think it would be nice to have at least one test in the Python side, as @yaochengji suggested.
Other than that, LGTM.
Lower cummin op. Fixes #8231.
Reused most of the changes from #8491 except for using min and argmin.