Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide Tensor Padding Helpers #960 #1097

Merged
merged 22 commits into from Mar 27, 2024
Merged

Provide Tensor Padding Helpers #960 #1097

merged 22 commits into from Mar 27, 2024

Conversation

jcmullwh
Copy link
Contributor

@jcmullwh jcmullwh commented Dec 24, 2023

Pull Request for Tensor Padding Feature Enhancement

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Fixes #960

Changes

This PR addresses the need for padding functionality in tensors of float and integer types. This is an initial implementation covering only constant value padding. The solution implemented involves adding methods for both constant padding mode and handling different padding sizes. The changes enhance the library's functionality by enabling users to easily manipulate tensor dimensions. The functionality is largely based on pytorch listed as an example.

Testing

The testing for these changes involves unit tests added to the tensor API testing module. These tests cover multiple scenarios including padding 2D and 4D tensors with constant values, and applying asymmetric padding. The tests ensure the correct implementation of padding across different tensor types and dimensions. All new tests have been executed successfully with the existing test suite, confirming that the new feature works as expected.

Remaining Work

#1535

Create padding implementation for the last two dimensions of Float and Int Tensors.

Create PadMode Enum, allowing Constant padding.

Create Padding Struct with Uniform, Asymmetric, height, and width implementations.

Create tests for the padding implementation.
remove unneeded import
Use crate Element

Swap from old from_data() to new from_data_devauto()
Formatting changes from cargo fmt --all
One more format change that cargo fmt didn't get the first time.
Modify Example to ensure it works.
better names for impl / input variables.
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I primarily reviewed the API, but there are still some fixes to be made regarding remaining prints and comments.

Comment on lines 606 to 612
// Create the padded tensor
let padded_shape = Shape::from(padded_dims);
let padded_data = Data::full(padded_shape, padding_value);
let padded_tensor = Tensor::<B, D, K>::from_data_devauto(padded_data);

// Assign the original tensor data to the appropriate slice of the padded tensor
padded_tensor.slice_assign(ranges, self)
Copy link
Member

Choose a reason for hiding this comment

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

Using Data is not efficient; it's preferable to use Tensor::full(shape, value) instead of Data::full.

pub fn pad(
self,
pad_amount: Padding,
pad_mode: PadMode,
Copy link
Member

Choose a reason for hiding this comment

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

I would change a bit the API

Padding => PadSize.

The mode should contains the value:

let tensor = tensor.pad(PadSize::uniform(2), PadMode::Constant(0));

}

/// Represents padding quantity for a tensor.
pub struct Padding {
Copy link
Member

Choose a reason for hiding this comment

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

PadSize

- Change Padding to PadSize.
- integrate padding value into PadMode.
- update tests and examples.
Improve comments+naming and remove println
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

Just some minor comments to be updated with the new changes added to the API.

burn-tensor/src/tensor/api/base.rs Outdated Show resolved Hide resolved
burn-tensor/src/tensor/api/base.rs Outdated Show resolved Hide resolved
burn-tensor/src/tensor/api/base.rs Outdated Show resolved Hide resolved
@antimora
Copy link
Collaborator

Let us know if you need more assistance with this PR.

@jcmullwh
Copy link
Contributor Author

Let us know if you need more assistance with this PR.

Got bogged down the last couple weeks, should be able to get it fixed this weekend!

@antimora antimora added the feature The feature request label Jan 31, 2024
Moved pad to numeric

Simplified PadMode Element

updated tensor creations

fixed doc example
@antimora antimora added the stale The issue or pr has been open for too long label Feb 24, 2024
@github-actions github-actions bot removed the stale The issue or pr has been open for too long label Mar 1, 2024
@antimora antimora self-assigned this Mar 19, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.34%. Comparing base (40a26bd) to head (4fe498c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1097      +/-   ##
==========================================
+ Coverage   86.32%   86.34%   +0.02%     
==========================================
  Files         680      681       +1     
  Lines       77647    77761     +114     
==========================================
+ Hits        67030    67145     +115     
+ Misses      10617    10616       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antimora
Copy link
Collaborator

@nathanielsimard, I have cleaned up this PR and resolved all the issues so we can merge it. The remaining work (bool tensor) is in a separate ticket #1535 (assigned to you)

Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

LGTM! Apart from the existing review comment I only have one minor comment regarding parameter naming but I'm approving in advance.

crates/burn-tensor/src/tensor/api/numeric.rs Outdated Show resolved Hide resolved
@antimora antimora merged commit 626457e into tracel-ai:main Mar 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature The feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Tensor Padding Helpers
4 participants