Skip to content
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

ReverseSequence op #1177

Merged
merged 7 commits into from
Apr 23, 2022
Merged

ReverseSequence op #1177

merged 7 commits into from
Apr 23, 2022

Conversation

CharlieL7
Copy link
Collaborator

Implements the ReverseSequence ONNX operator as a parser.

  • This parser can only handle a constant sequence_lens input. This is the same as what is handled for TensorRT as far as I can tell.
  • We could handle a variable sequence_lens input; that would require ref and GPU implementations of the operator.
  • The ONNX backend tests are disabled because this does not handle variable sequence_lens.

@CharlieL7 CharlieL7 requested review from kahmed10 and turneram April 14, 2022 19:50
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #1177 (10bd32e) into develop (764273e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 10bd32e differs from pull request most recent head d548488. Consider uploading reports for the commit d548488 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1177      +/-   ##
===========================================
- Coverage    92.59%   92.59%   -0.01%     
===========================================
  Files          431      432       +1     
  Lines        13877    13687     -190     
===========================================
- Hits         12850    12673     -177     
+ Misses        1027     1014      -13     
Impacted Files Coverage Δ
src/onnx/parse_reversesequence.cpp 100.00% <100.00%> (ø)
src/dynamic_loader.cpp 77.77% <0.00%> (-1.17%) ⬇️
src/include/migraphx/value.hpp 94.44% <0.00%> (-1.16%) ⬇️
src/include/migraphx/op/if_op.hpp 88.88% <0.00%> (-1.12%) ⬇️
src/include/migraphx/op/quant_convolution.hpp 90.32% <0.00%> (-1.11%) ⬇️
src/include/migraphx/op/convolution.hpp 90.62% <0.00%> (-1.05%) ⬇️
src/onnx/parse_shape.cpp 90.00% <0.00%> (-0.91%) ⬇️
src/targets/cpu/eltwise.cpp 87.50% <0.00%> (-0.74%) ⬇️
src/quantization.cpp 85.00% <0.00%> (-0.72%) ⬇️
src/process.cpp 82.75% <0.00%> (-0.58%) ⬇️
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 764273e...d548488. Read the comment docs.

instruction_ref s0;
if(sequence_lens[b] > 1)
{
s0 = info.add_instruction(make_op("slice",
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like most of the fields for the operator are the same with slight variations. Maybe you can make a helper function add_slice() where the starts and ends are modified for each call?

instruction_ref s0;
if(sequence_lens[b] > 1)
{
s0 = info.add_instruction(make_op("slice",
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like most of the fields for the operator are the same with slight variations. Maybe you can make a helper function add_slice() where the starts and ends are modified for each call?

input);
for(int b = 1; b < batch_size; ++b)
{
auto s0 = mm->add_instruction(migraphx::make_op("slice",
Copy link
Collaborator

Choose a reason for hiding this comment

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

same feedback as I mentioned in the parser.

input);
for(int b = 1; b < batch_size; ++b)
{
auto s0 = mm->add_instruction(migraphx::make_op("slice",
Copy link
Collaborator

Choose a reason for hiding this comment

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

same feedback as I mentioned in the parser.

@causten causten merged commit 3190678 into develop Apr 23, 2022
@causten causten deleted the reversesequence_op branch April 23, 2022 03:50
@kahmed10 kahmed10 mentioned this pull request Jul 12, 2022
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.

4 participants