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

Fix for test_add #191

Merged
merged 2 commits into from
Mar 22, 2019
Merged

Conversation

Cdebus
Copy link
Contributor

@Cdebus Cdebus commented Mar 21, 2019

Fixes #164

  • Error was result of uncomplete cleanup after restructuring of binary operator generic function
  • removal of faulty tensor
  • tests on NotImplementedError raise are performed with T_s

- removal of faulty tensor
- tests on NotImplementedError raise are performed with T_s
@codecov-io
Copy link

codecov-io commented Mar 21, 2019

Codecov Report

Merging #191 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #191      +/-   ##
=========================================
- Coverage   89.61%   89.6%   -0.01%     
=========================================
  Files          36      36              
  Lines        3639    3637       -2     
=========================================
- Hits         3261    3259       -2     
  Misses        378     378
Impacted Files Coverage Δ
heat/core/tests/test_arithmetics.py 100% <100%> (ø) ⬆️

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 e31ac4f...03a9a79. Read the comment docs.

@Cdebus Cdebus requested a review from coquelin77 March 21, 2019 10:06
@coquelin77
Copy link
Member

everything passes here with up to 4 processes.

I was looking through the test though and I found a few stylistic things. Such as I didnt see why when you copy the T1 tensor to form T_s you say you want comm=None. I changed that to T1.comm and I also changed the device to T1.device. I also did some pep8 formating in the two lines following.

T_s = ht.tensor(T1._tensor__array, T1.shape, T1.dtype, 0, T1.device, T1.comm)
Ts = ht.ones((2, 2), split=1)
otherType = (2, 2)

@Cdebus
Copy link
Contributor Author

Cdebus commented Mar 22, 2019

Well, to be honest, there is no reason^^.... I didn't think about this at the time, I only cared for it to be a split tensor. But it does make a lot more sense to copy the device and communication. Thanks for catching it!

@coquelin77 coquelin77 merged commit d787441 into helmholtz-analytics:master Mar 22, 2019
@Cdebus Cdebus deleted the 164-Bugfix_test_add branch April 7, 2020 11:05
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.

3 participants