Skip to content

Mul, Sub and Add Layer with broadcasting support#186

Merged
allnes merged 12 commits into
mainfrom
Semyon1104/Mul_layer
Aug 4, 2025
Merged

Mul, Sub and Add Layer with broadcasting support#186
allnes merged 12 commits into
mainfrom
Semyon1104/Mul_layer

Conversation

@Semyon1104

Copy link
Copy Markdown
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

Copilot AI left a comment

Copy link
Copy Markdown

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) {

Comment thread src/layers/BinaryOpLayer.cpp
Comment thread src/layers/BinaryOpLayer.cpp
Comment thread test/single_layer/test_binaryoplayer.cpp

namespace itlab_2023 {

class BinaryOpLayer : public Layer {

Copy link
Copy Markdown
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

@Semyon1104 Semyon1104 Aug 3, 2025

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

Comment thread include/layers/BinaryOpLayer.hpp Outdated
@allnes

allnes commented Aug 3, 2025

Copy link
Copy Markdown
Member

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

@allnes

allnes commented Aug 3, 2025

Copy link
Copy Markdown
Member

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

@allnes

allnes commented Aug 3, 2025

Copy link
Copy Markdown
Member

@aobolensk

Comment thread include/layers/BinaryOpLayer.hpp Outdated
Comment thread src/layers/BinaryOpLayer.cpp Outdated
Comment thread test/single_layer/test_binaryoplayer.cpp Outdated
Comment thread include/layers/BinaryOpLayer.hpp Outdated
@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