Skip to content

Conversation

@Semyon1104
Copy link
Collaborator

I will also add the functionality of element-by-element subtraction and element-by-element addition layers - sub and add

@Semyon1104 Semyon1104 changed the title MulLayer with broadcasting support Mul, Sub and Add Layer with broadcasting support Jul 25, 2025
@allnes allnes requested a review from Copilot July 27, 2025 17:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a BinaryOpLayer class that supports element-wise multiplication, addition, and subtraction operations with broadcasting support for tensors. It extends existing layer functionality to handle binary operations between tensors of different but compatible shapes.

Key changes:

  • New BinaryOpLayer class with support for multiplication, addition, and subtraction operations
  • Broadcasting logic to handle tensors with compatible but different shapes
  • Scalar tensor operations for element-wise operations with single values

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
include/layers/BinaryOpLayer.hpp Header file defining the BinaryOpLayer class interface and operation enums
src/layers/BinaryOpLayer.cpp Implementation of binary operations with broadcasting and scalar support
include/layers/Shape.hpp Added equality operators for Shape class to support testing
test/single_layer/test_binaryoplayer.cpp Comprehensive test suite covering various broadcasting scenarios and operations
Comments suppressed due to low confidence (2)

test/single_layer/test_binaryoplayer.cpp:180

  • Test is named 'BroadcastingTestAdd' but is part of the 'BinaryOpLayerMulTests' test fixture. Consider creating a separate test fixture for Add operations or renaming to reflect the actual operation being tested.
TEST_F(BinaryOpLayerMulTests, BroadcastingTestAdd) {

test/single_layer/test_binaryoplayer.cpp:204

  • Test is named 'BroadcastingTestSubGooglNet' but is part of the 'BinaryOpLayerMulTests' test fixture. Consider creating a separate test fixture for Sub operations or renaming to reflect the actual operation being tested.
TEST_F(BinaryOpLayerMulTests, BroadcastingTestSubGooglNet) {


namespace itlab_2023 {

class BinaryOpLayer : 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.

It's part of elementwise operation

Copy link
Collaborator Author

@Semyon1104 Semyon1104 Aug 3, 2025

Choose a reason for hiding this comment

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

I thought it would be better to put the logic of interaction between two tensors with a fairly comprehensive broadcast logic in an another layer to separate the situations where the input is a single tensor, EWLayer, and where there are two input arrays and 1 output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or is it better to combine these layers into one?

Copy link
Member

Choose a reason for hiding this comment

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

Ok we can choose compromise, please create operation - Logic and unite all logic operations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean to add the kDiv operation and name the enumerated type logic?

@allnes
Copy link
Member

allnes commented Aug 3, 2025

The current implementation recalculates indices for each element. Consider caching stride calculations for better performance with large tensors.

@allnes
Copy link
Member

allnes commented Aug 3, 2025

While Mul, Add, and Sub are implemented, division would complete the basic arithmetic set. Consider adding kDiv to the BinaryOpType enum.

@allnes
Copy link
Member

allnes commented Aug 3, 2025

@aobolensk

@aobolensk aobolensk requested a review from allnes August 4, 2025 13:19
@allnes allnes enabled auto-merge (squash) August 4, 2025 13:21
@allnes allnes disabled auto-merge August 4, 2025 13:21
@allnes allnes merged commit 7509f2f into main Aug 4, 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