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

Ryanunderhill/concat neg axis #152

Merged
merged 5 commits into from
Dec 13, 2018
Merged

Conversation

RyanUnderhill
Copy link
Member

Add negative axis support to concat

@RyanUnderhill RyanUnderhill requested a review from a team as a code owner December 11, 2018 22:20
@RyanUnderhill RyanUnderhill requested a review from snnn December 12, 2018 01:51
@snnn
Copy link
Member

snnn commented Dec 13, 2018

Does the spec say the axis can be negative ?

@@ -36,19 +39,19 @@ Status ConcatBase::PrepareForCompute(OpKernelContext* ctx, int input_count, Prep
for (int index = 0; index < input_count; index++) {
tensor_pointer = ctx->Input<Tensor>(index);
if (tensor_pointer == nullptr) return Status(common::ONNXRUNTIME, common::FAIL, "input count mismatch");
concat_axis_size += tensor_pointer->Shape()[int(axis_)];
concat_axis_size += tensor_pointer->Shape()[int(axis)];
Copy link
Member

Choose a reason for hiding this comment

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

why cast axis to in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the operator[] for Shape() didn't take int64_t initially so it used to give errors. That was back when it was first written. I can change it and see if you want me to.

@@ -63,7 +66,7 @@ Status ConcatBase::PrepareForCompute(OpKernelContext* ctx, int input_count, Prep

// The input_axis_pitch is the number of elements to add to move to the next split axis in the input
int64_t input_axis_pitch = 1;
for (int i = int(data_n.Shape().NumDimensions()); i-- > axis_;)
for (int i = int(data_n.Shape().NumDimensions()); i-- > axis;)
Copy link
Member

Choose a reason for hiding this comment

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

Would static_cast(...) be better than int(...) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe 'auto i' would be better. Is there a difference in this case?

Copy link
Member

Choose a reason for hiding this comment

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

No difference.
It doesn't conform our coding style:
https://google.github.io/styleguide/cppguide.html#Casting

@RyanUnderhill RyanUnderhill merged commit 30ab01e into master Dec 13, 2018
@RyanUnderhill RyanUnderhill deleted the ryanunderhill/concat_neg_axis branch December 13, 2018 22:43
souptc pushed a commit that referenced this pull request Dec 17, 2018
Another set of simple ops

Plus change the ceil/floor implementations to use Eigen since I figured out a way

Related work items: #152
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