Skip to content

Conversation

@Semyon1104
Copy link
Collaborator

No description provided.

Comment on lines +55 to +63
size_t outer_size = 1;
for (int i = 0; i < axis; ++i) {
outer_size *= shape[i];
}

size_t inner_size = 1;
for (size_t i = axis + 1; i < shape.dims(); ++i) {
inner_size *= shape[i];
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider using std::accumulate

size_t outer_size = std::accumulate(
    shape.begin(), shape.begin() + axis, 
    static_cast<size_t>(1), std::multiplies<size_t>());

size_t inner_size = std::accumulate(
    shape.begin() + axis + 1, shape.end(), 
    static_cast<size_t>(1), std::multiplies<size_t>());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we do not have access to the vector<size_t> dims_ inside the Shape class, as it is a private field. and therefore we cannot use iterators

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's leave as is

private:
int axis_;
std::vector<int> splits_;
int num_outputs_ = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int num_outputs_ = 0;
int num_outputs_;

}

int SplitLayer::get_normalized_axis(int rank) const {
if (axis_ < 0) return axis_ + rank;
Copy link
Member

Choose a reason for hiding this comment

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

This still can have underflow. For example: axis = -5, rank = 2

Comment on lines 49 to 53
const auto& part_sizes =
splits_.empty()
? std::vector<int>(num_outputs_,
static_cast<int>(shape[axis]) / num_outputs_)
: splits_;
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is for the ability to split into equal parts if splits_ are not specified

#include "layers/Tensor.hpp"

using namespace it_lab_ai;

Copy link
Member

Choose a reason for hiding this comment

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

Please, add negative axis tests

@allnes
Copy link
Member

allnes commented Aug 14, 2025

Use block copies where memory is contiguous. For each output slice at fixed outer/part, the data region is contiguous (output_axis_size * inner_size elements). Replace the inner element-wise loops with a single std::copy_n (or memcpy when types allow). This will significantly reduce overhead on larger tensors.

@allnes
Copy link
Member

allnes commented Aug 14, 2025

Make the mode explicit. splits_ vs num_outputs_ are mutually exclusive. Consider:
• std::variant<std::vector, int> split_spec_; or
• std::optional num_outputs_;
This encodes invariants in the type system and simplifies validation.

@allnes
Copy link
Member

allnes commented Aug 14, 2025

Tests to add
• Uneven split (remainder) test. Verify that the entire remainder lands in the last output (per the chosen policy).
• num_outputs_ > size test. Either expect an exception (if you forbid it) or assert the exact shapes including zero-sized outputs (if you allow it).
• Integer dtype path. Add at least one test covering Type::kInt to ensure both code paths behave identically.

@allnes
Copy link
Member

allnes commented Aug 14, 2025

Behavior when num_outputs_ > size_along_axis is undefined.
Today this yields multiple zero-sized outputs. Decide and enforce one policy:
• either reject with a clear exception in validation, or
• allow but document it and add tests asserting exact output shapes (including zeros).
Leaving this implicit can easily break downstream code that doesn’t expect zero-length tensors.

@aobolensk aobolensk merged commit 4e5a6c6 into main Aug 15, 2025
19 checks passed
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