Replacing get_shape() #16786
Open
Replacing get_shape() #16786
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbedichekTT
requested review from
razorback3,
dongjin-na,
cfjchu,
ayerofieiev-tt and
dmakoviichuk-tt
as code owners
ttnn/cpp/ttnn/operations/moreh/moreh_cumsum/device/moreh_cumsum_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_cumsum/device/moreh_cumsum_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_cumsum/device/moreh_cumsum_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_dot_backward/device/moreh_dot_backward_program_factory.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_dot_backward/device/moreh_dot_backward_program_factory.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
ttnn/cpp/ttnn/operations/moreh/moreh_getitem/device/moreh_getitem_device_operation.cpp
Outdated
Show resolved
Hide resolved
| for (int i = 0; i < input_shape.rank(); ++i) { | ||
| TT_FATAL( | ||
| input_shape[i] == output_shape[i], | ||
| "Input shape must match output shape. Received input_shape = {} and output_shape = {}.", | ||
| input_shape[i], | ||
| output_shape[i]); | ||
| TT_FATAL( |
There was a problem hiding this comment.
@razorback3 can someone from the team please check?
Comment on lines
24
to
26
| // Unnecessary? -> | ||
| //const auto input_shape_wo_padding = input.get_logical_shape(); | ||
| //const auto output_shape_wo_padding = input.get_logical_shape(); |
There was a problem hiding this comment.
My recommendation is to remove things you believe are not needed, instead of commenting them
You can then ping related parties in the PR to clarify with them.
Comment on lines
119
to
120
| uint32_t index_size = index.get_logical_shape()[-1]; | ||
| uint32_t index_size_without_padding = index.get_logical_shape()[-1]; |
There was a problem hiding this comment.
@razorback3 , both values are without a padding here. I think someone from Moreh team will have to take a look
| } | ||
|
|
||
| uint32_t index_size = index_tensors.front().get_shape().value[-1]; | ||
| const uint32_t& index_size = index_tensors.front().get_logical_shape()[-1]; |
There was a problem hiding this comment.
get_shape().value is returns a LegacyShape. its operator[] will give padded value.
So the proper replacement here is get_padded_shape()
Comment on lines
45
to
46
| const auto& input_shape = input.get_padded_shape(); | ||
| const auto& input_shape_without_padding = input.get_logical_shape() |
There was a problem hiding this comment.
@razorback3 , another place where there seem to be an issue. get_shape and input_shape.value.without_padding() are basically returning the same thing. One can probably be removed.
| auto input_shape_without_padding = input_shape.value.without_padding(); | ||
| auto output_shape = output.get_shape(); | ||
| auto output_shape_without_padding = output_shape.value.without_padding(); | ||
| const auto& input_shape = input.get_padded_shape(); |
There was a problem hiding this comment.
@jbedichekTT input_shape variable here seem to have had a semantics of get_logical_shape()
There was a problem hiding this comment.
auto input_shape = input.get_shape();
| @@ -31,34 +31,37 @@ void MorehGroupNormOperation::validate_tensors( | |||
| check_tensor(beta, "moreh_group_norm", "beta"); | |||
|
|
|||
| // input (N, C, H, W) | |||
| auto C = input.get_shape().value[1]; | |||
| auto C = input.get_logical_shape()[1]; | |||
| TT_FATAL(C % num_groups == 0, "input_shape[1] must be divisible by num_groups."); | ||
| // output (N, C, H, W) | ||
| if (output.has_value()) { | ||
| C = output.value().get_shape().value[1]; | ||
| C = output.value().get_logical_shape()[1]; |
| TT_FATAL(C % num_groups == 0, "beta_shape[-1] must be divisible by num_groups."); | ||
| } | ||
|
|
||
| // mean (1, 1, N, num_groups) | ||
| if (mean.has_value()) { | ||
| TT_FATAL( | ||
| mean.value().get_shape().value.without_padding()[-1] == num_groups, | ||
| //"mean_shape[-1] must match num_groups."); | ||
| mean.value().get_logical_shape()[-1] == num_groups, |
There was a problem hiding this comment.
mean->get_logical_shape()[-1]
| TT_FATAL(C % num_groups == 0, "beta_shape[-1] must be divisible by num_groups."); | ||
| } | ||
|
|
||
| // mean (1, 1, N, num_groups) | ||
| if (mean.has_value()) { | ||
| TT_FATAL( | ||
| mean.value().get_shape().value.without_padding()[-1] == num_groups, | ||
| //"mean_shape[-1] must match num_groups."); |
| TT_FATAL(C % num_groups == 0, "input_shape[1] must be divisible by num_groups."); | ||
| // input_grad (N, C, H, W) | ||
| if (input_grad.has_value()) { | ||
| C = input_grad.value().get_shape().value[1]; | ||
| C = input_grad.value().get_logical_shape()[1]; |
Comment on lines
68
to
69
| const auto& input_shape = input.get_padded_shape(); | ||
| const auto& input_shape_without_padding = input.get_logical_shape(); |
There was a problem hiding this comment.
this is flipped. I recommend to rename/refactor variable names.
input_shape --> input_shape_padded
input_shape_without_padding --> input_shape
Comment on lines
67
to
68
| const auto& mean_rstd_shape = mean.get_padded_shape(); | ||
| const auto mean_rstd_shape_without_padding = mean.get_logical_shape(); |
| const auto mean_rstd_shape = mean.get_shape().value; | ||
| const auto mean_rstd_shape_without_padding = mean_rstd_shape.without_padding(); | ||
|
|
||
| const auto& mean_rstd_shape_without_padding = mean.get_logical_shape(); |
Comment on lines
33
to
34
| const auto& input_wo_shape = input.get_logical_shape(); | ||
| const auto& other_wo_shape = other.get_logical_shape(); |
There was a problem hiding this comment.
should just be input_shape and other_shape
Comment on lines
121
to
122
| const auto& input_shape = tensor_args.input.get_padded_shape(); | ||
| const auto& other_shape = tensor_args.other.get_padded_shape(); |
There was a problem hiding this comment.
should be input_shape_padded and other_shape_padded
Comment on lines
126
to
127
| const auto& input_shape_wo_padding = tensor_args.input.get_logical_shape(); | ||
| const auto& other_shape_wo_padding = tensor_args.other.get_logical_shape(); |
There was a problem hiding this comment.
Suggested change
| const auto& input_shape_wo_padding = tensor_args.input.get_logical_shape(); | |
| const auto& other_shape_wo_padding = tensor_args.other.get_logical_shape(); | |
| const auto& input_shape = tensor_args.input.get_logical_shape(); | |
| const auto& other_shape = tensor_args.other.get_logical_shape(); |
Comment on lines
54
to
55
| const auto& a_shape = tensor_a.get_logical_shape(); | ||
| const auto& b_shape = tensor_b.get_logical_shape(); |
Comment on lines
140
to
141
| const auto& input_shape = input.get_padded_shape(); | ||
| const auto& input_shape_wo_padding = input.get_logical_shape(); |
| auto rank = shape.rank(); | ||
| // Replace? -> | ||
| auto padding = shape.value.padding(); |
There was a problem hiding this comment.
this likely does not compile.
Please revert this change. Someone will later make a pass here.
CC @sminakov-tt
| const auto& input_grad_shape = input_grad.get_shape(); | ||
| const auto& input_grad_shape_wo_padding = input_grad_shape.value.without_padding(); | ||
| const auto& input_grad_shape = input_grad.get_logical_shape(); | ||
| //const auto& input_grad_shape_wo_padding = input_grad_shape.value.without_padding(); |
There was a problem hiding this comment.
why is this file changed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
None
Problem description
Replacing the legacy Tensor::get_shape() with new Tensor::get_padded_shape and Tensor::get_logical_shape from Moreh folder.
What's changed
Checklist