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
Add Split-20 - change uneven split behavior to be more torch-like #5321
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
onnx/defs/tensor/defs.cc
Outdated
"Uneven split mode. " | ||
"Possible values are 'torch' (default) and 'legacy'.", | ||
AttributeProto::STRING, | ||
std::string("torch")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the legacy as default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know torch is the desired one, and I set legacy to be default in version converter when coming from opset 19 to 20. I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think the current choice is ok. Especially if it is only the torch exporter that uses this, and presumably the torch converter needs the new option, not the old one.
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
split.push_back(chunk_size); | ||
} | ||
} else { | ||
int chunk_size = (split_dim_value / num_outputs) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this doesn't always work. Eg., if we split 7 values into 5, then this will produce 4, 4, 4, 4, -1 which doesn't make sense. We should do 4, 3, 0, 0, 0, I think. We may need to change the description accordingly. In the other mode, we will do 2, 2, 1, 1, 1.
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
onnx/backend/test/case/node/split.py
Outdated
"Split", | ||
inputs=["input"], | ||
outputs=["output_1", "output_2", "output_3"], | ||
axis=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the axis
, it would make sense to change the new attribute introduced minimize_diff
or whatever it is called. At least, I am looking for some test-case to show the difference that attribute makes, whether that is this test or some other test.
Hi, is this ready for review? Would it be possible to complete this PR for the upcoming release? |
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: p-wysocki <przemyslaw.wysocki@intel.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Added new tests, generated docs and test files are on the way - I'm currently having some issues with protobuf in my environment, which stops me from generating them. |
It will also have to be moved to a later opset (21, I guess?) |
Spec like We can make it more specific by:
|
Description
Introduce Split-20, which performs uneven tensor split in torch's manner: instead of reducing only the size of the last dimension of the output, it reduces the last n dimensions' sized by 1 each.
This was done using a
mode
attribute, which has 2 possible values:torch
(default) andlegacy
. It's up to discussion if it should stay like that or maybe it should be renamed or changed to a boolean.Motivation and Context
Issue: #4742
TODO
test_split_uneven_split_3d_4
)