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

Remainder operator #1726

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remainder operator #1726

wants to merge 2 commits into from

Conversation

kantic
Copy link

@kantic kantic commented May 5, 2024

Pull Request Template

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

Related PR: 1597
Related Issue: 1510

Changes

Problem: The tensor.remainder_scalar() method has been implemented in a previous PR. However, its not possible to use the % operator, e.g., tensor % 2.0.
Changes: Added ops::Rem trait implementation for Tensor.

Testing

Added a test for Tensor that makes use of the % operator.

Copy link
Collaborator

@antimora antimora left a comment

Choose a reason for hiding this comment

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

Thank you for enhancing the code.

The code looks good, but I have noticed we are missing an entry in Burn book. Can you please add this documentation here: burn-book/src/building-blocks/tensor.md and you also mention about your new % operator as well.

@antimora antimora requested a review from laggui May 5, 2024 20:11
Copy link

codecov bot commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.47%. Comparing base (152509c) to head (f795936).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1726   +/-   ##
=======================================
  Coverage   86.47%   86.47%           
=======================================
  Files         698      698           
  Lines       82874    82888   +14     
=======================================
+ Hits        71667    71681   +14     
  Misses      11207    11207           

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

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.

LGTM

Once the book is updated we can merge it!

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.

None yet

4 participants