Skip to content

Conversation

@Semyon1104
Copy link
Collaborator

No description provided.

#include "layers/Layer.hpp"
#include "layers/Tensor.hpp"

namespace itlab_2023 {
Copy link
Member

Choose a reason for hiding this comment

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

please change namespace as in the last PR


namespace itlab_2023 {

class ReduceSumLayer : public Layer {
Copy link
Member

Choose a reason for hiding this comment

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

Please use Reduce op and inside it use sum, avg, mult etc

@allnes
Copy link
Member

allnes commented Aug 3, 2025

Uses 1-based indexing which differs from NumPy/PyTorch conventions (0-based). This could confuse users familiar with other frameworks. Recommendation: Consider switching to 0-based indexing.

@allnes
Copy link
Member

allnes commented Aug 3, 2025

Add Negative Axis Support

  // In normalize_axes
  if (axis < 0) {
      axis += input_rank;  // Convert negative to positive
  }

@allnes
Copy link
Member

allnes commented Aug 3, 2025

@aobolensk

Copy link
Member

@aobolensk aobolensk left a comment

Choose a reason for hiding this comment

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

Why do we enumerate axis from 1 instead of 0?

Comment on lines 138 to 144
for (size_t i = input_rank - 1;; --i) {
++in_coords[i];
if (in_coords[i] < input_shape[i] || i == 0) {
break;
}
in_coords[i] = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

This loop is risky, you check that i > 0 after accessing i-th element.

@allnes allnes requested a review from aobolensk August 5, 2025 20:37
@aobolensk aobolensk enabled auto-merge (squash) August 6, 2025 06:25
@aobolensk aobolensk disabled auto-merge August 6, 2025 06:25
@aobolensk aobolensk merged commit 499c1e7 into main Aug 6, 2025
17 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