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

Implementing right hand side operations #204

Merged
merged 13 commits into from
Jun 4, 2019

Conversation

TheSlimvReal
Copy link
Contributor

Target #168.

Added right-hand-side implementations for all implemented operations like "/", "%", ...

@Markus-Goetz Markus-Goetz requested a review from Cdebus April 4, 2019 15:49
@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

Merging #204 into master will increase coverage by 0.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #204      +/-   ##
=========================================
+ Coverage   96.28%   96.3%   +0.01%     
=========================================
  Files          47      47              
  Lines        6731    6762      +31     
=========================================
+ Hits         6481    6512      +31     
  Misses        250     250
Impacted Files Coverage Δ
heat/core/dndarray.py 89.68% <100%> (+0.67%) ⬆️
heat/core/arithmetics.py 100% <100%> (ø) ⬆️
heat/core/tests/test_arithmetics.py 99.06% <86.66%> (-0.94%) ⬇️
heat/core/operations.py 93.7% <0%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b345f16...c70b11d. Read the comment docs.

…hand_side_operators

# Conflicts:
#	heat/core/tests/test_tensor.py
('__mod__', operator.mod, False),
('__pow__', operator.pow, False)
)
tensor = ht.float32([[1, 4], [2, 3]])
Copy link
Member

Choose a reason for hiding this comment

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

We need test cases here for split tensors

Copy link
Contributor

Choose a reason for hiding this comment

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

This is related to #167, which needs to be fixed first
At the moment, binary operations do not work properly for split tensors.

__radd__ = __add__
__rmul__ = __mul__

# __rfloordiv__ // TODO: Implement me when implementing __floordiv__
Copy link
Member

Choose a reason for hiding this comment

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

Please provide both implementations (regular and right-hand side floor-div). Straight forward by adapting div (torch supports truediv as well)

@@ -451,41 +451,93 @@ def __truediv__(self, other):
"""
return arithmetics.div(self, other)

def __rtruediv__(self, other):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a technicality, but shouldn't the functions be sorted alphabetically? (at least for the operators before it is the case)
EDIT: This goes for all the operators below

@TheSlimvReal TheSlimvReal requested a review from Markus-Goetz May 7, 2019 06:11
@TheSlimvReal TheSlimvReal force-pushed the features/168-right_hand_side_operators branch from c16680c to a1f4a8f Compare May 7, 2019 11:28
@TheSlimvReal TheSlimvReal requested a review from Cdebus May 15, 2019 18:29
@Markus-Goetz
Copy link
Member

Bump, please update to the current master, would want to merge :)

@Markus-Goetz Markus-Goetz merged commit 37c0619 into master Jun 4, 2019
@Markus-Goetz Markus-Goetz deleted the features/168-right_hand_side_operators branch June 4, 2019 07:34
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