enable 1-D axis in cumsum #17650
enable 1-D axis in cumsum #17650
Conversation
| axis = inputs.at(1); // optional input, 0-D tensor | ||
| // optional input, 0-D or 1-D tensor | ||
| const auto& axis_shape = inputs.at(1).get_partial_shape(); | ||
| NGRAPH_CHECK(axis_shape.rank().get_length() == 0 || axis_shape.rank().get_length() == 1, |
There was a problem hiding this comment.
There's no axis input check in the core operator intentionally so I'd rather not add it here either https://github.com/openvinotoolkit/openvino/blob/master/src/core/src/op/cum_sum.cpp#L47. @mitruska any thoughts?
There was a problem hiding this comment.
I would recommend to use interpret_as_scalar helper as a common solution for such cases. It also has corresponding checks. Example:
| const auto& axis_shape = inputs.at(1).get_partial_shape(); | ||
| NGRAPH_CHECK(axis_shape.rank().get_length() == 0 || axis_shape.rank().get_length() == 1, | ||
| "axis shape should be 0-D or 1-D."); | ||
| axis = |
There was a problem hiding this comment.
Although ONNX only allows scalar axis input I think this is ok. I'd like to see it demonstrated with even a single test though. Please add it to this file https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/onnx/tests/onnx_import.in.cpp#L621 and use any of the existing CumSum tests as a template.
| axis = inputs.at(1); // optional input, 0-D tensor | ||
| // optional input, 0-D or 1-D tensor | ||
| const auto& axis_shape = inputs.at(1).get_partial_shape(); | ||
| NGRAPH_CHECK(axis_shape.rank().get_length() == 0 || axis_shape.rank().get_length() == 1, |
There was a problem hiding this comment.
I would recommend to use interpret_as_scalar helper as a common solution for such cases. It also has corresponding checks. Example:
| NGRAPH_CHECK(axis_shape.rank().get_length() == 0 || axis_shape.rank().get_length() == 1, | ||
| "axis shape should be 0-D or 1-D."); | ||
| axis = | ||
| axis_shape.rank().get_length() == 0 ? inputs.at(1) : std::make_shared<default_opset::Squeeze>(inputs.at(1)); |
There was a problem hiding this comment.
To avoid issues get_legth() shouldn't be called without dynamism check,
my suggestion is to check the shape and call the helper from #include "utils/reshape.hpp":
| axis_shape.rank().get_length() == 0 ? inputs.at(1) : std::make_shared<default_opset::Squeeze>(inputs.at(1)); | |
| axis_shape.is_dynamic() ? inputs.at(1) : reshape::interpret_as_scalar(inputs.at(1)); |
|
This PR will be closed in a week because of 2 weeks of no activity. |
|
@xczhai Can we look into the review feedback and address these? Thanks! |
|
fix according to the comments. |
mitruska
dismissed
tomdol’s stale review
Comments are applied, dismissed due to absence of the reviewer.
Details:
Tickets: