Skip to content

support cumprod op #2133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 22, 2023
Merged

support cumprod op #2133

merged 7 commits into from
Mar 22, 2023

Conversation

f-salvetti
Copy link
Contributor

Supports cumprod TF op. Both reverse and exclusive flags are supported.

Signed-off-by: Francesco Salvetti <[email protected]>
Signed-off-by: Francesco Salvetti <[email protected]>
This reverts commit 6ad3231.

Signed-off-by: Francesco Salvetti <[email protected]>
Signed-off-by: Francesco Salvetti <[email protected]>
Signed-off-by: Francesco Salvetti <[email protected]>
Signed-off-by: Francesco Salvetti <[email protected]>
@f-salvetti f-salvetti marked this pull request as ready for review March 10, 2023 16:50
pad_const_node = one_node
pad_node = graph.make_node("Pad", inputs=[slice_node.output[0], pads_node.output[0], pad_const_node.output[0]],
op_name_scope=loop_name, outputs=[utils.make_name("pad")])
mul_node = graph.make_node("Mul", inputs=[graph.input_names[4], pad_node.output[0]],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to leverage reduce_prod to avoid multiplying those values one by one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, didn't think about that. I will refactor the code in order to use that operator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fatcat-z Actually, using ReduceProd, would require to loop over slices of the input tensor to be reduced and then concatenate results to create the output tensor. However, in order to support a non-constant axis for the CumProd op as in TF, I don't think it is possible to use the Concat op, since it requires the axis as an attribute. Due to this limitation, I don't see how I can use ReduceProd instead of Mul.

@fatcat-z
Copy link
Collaborator

The 2 CI failures are not supposed to be relative to your changes. I will figure them out.

Copy link
Collaborator

@fatcat-z fatcat-z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contributions.

@fatcat-z fatcat-z merged commit de939f3 into onnx:main Mar 22, 2023
@f-salvetti f-salvetti deleted the support-cumprod-op branch June 29, 2023 10:35
f-salvetti added a commit to f-salvetti/tensorflow-onnx that referenced this pull request Jul 4, 2023
- update current opset to 18
- added Cumprod (onnx#2133)
- added ReduceJoin (onnx#2091)

Signed-off-by: f-salvetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants