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

Features/125 modf #402

Merged
merged 50 commits into from
Dec 9, 2019
Merged

Features/125 modf #402

merged 50 commits into from
Dec 9, 2019

Conversation

lenablind
Copy link
Collaborator

Description

Concerned issues:
#125 (modf),
#126 (round)


#125 modf

  • rounding.py:
    Implementation of modf (following the example set by numpy)
  • dndarray.py:
    Addition of the function to be used as an attribute
  • test_rounding.py:
    Test function for modf

#126 round

  • rounding.py:
    Implementation of round
  • dndarray.py:
    Addition of the function to be used as an attribute
  • test_rounding.py:
    Test function for round

Special dependencies required: None, same as for heat

Fixes: #125 (modf), #126 (round)

Changes proposed:

Type of change

Select relevant options.

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation update

Are all split configurations tested and accounted for?
[ ] yes [x] no
Does this change require a documentation update outside of the changes proposed?
[ ] yes [x] no
Does this change modify the behaviour of other functions?
[ ] yes [x] no
Are there code practices which require justification?
[x] yes [ ] no

Copy link
Member

@coquelin77 coquelin77 left a comment

Choose a reason for hiding this comment

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

The tests complete successful at this time on my local machine. There may be an issue with travis...again

tensor([ 0.0000, -0.6000, -0.2000, -0.8000, -0.4000, 0.0000, 0.4000, 0.8000, 0.2000, 0.6000]))
"""

integralParts = ht.trunc(x)
Copy link
Member

Choose a reason for hiding this comment

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

dont need ht.trunc, should just be trunc(x)

in the future please avoid using ht.function() as it causes a cyclic import. instead use the form: file.function()

@@ -1,10 +1,11 @@
import torch
import heat as ht
Copy link
Member

Choose a reason for hiding this comment

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

cyclic import

self.assertIsInstance(float64_modf[1], ht.DNDarray)
self.assertEqual(float64_modf[0].dtype, ht.float64)
self.assertEqual(float64_modf[1].dtype, ht.float64)
self.assertTrue((x for x in float32_modf[0]._DNDarray__array) == y for y in comparison[0])
Copy link
Member

Choose a reason for hiding this comment

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

while technically correct, a neater way to do this might be to use ht.all(tensor == ht.array(comparison)) or torch.all as you use in the test_round function

@codecov
Copy link

codecov bot commented Nov 8, 2019

Codecov Report

Merging #402 into master will increase coverage by 0.01%.
The diff coverage is 99.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
+ Coverage   98.05%   98.07%   +0.01%     
==========================================
  Files          55       55              
  Lines       11285    11414     +129     
==========================================
+ Hits        11066    11194     +128     
- Misses        219      220       +1
Impacted Files Coverage Δ
heat/core/tests/test_suites/basic_test.py 100% <ø> (ø) ⬆️
heat/core/dndarray.py 95.66% <100%> (+0.03%) ⬆️
heat/core/tests/test_rounding.py 100% <100%> (ø) ⬆️
heat/core/rounding.py 95.08% <96.55%> (+1.14%) ⬆️

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 84587e3...2b52855. Read the comment docs.

@coquelin77
Copy link
Member

coquelin77 commented Nov 8, 2019

bump, coverage needs to be a bit higher. if you need help seeing where let me know, sometimes the codecov site doesnt show things right away

@lenablind
Copy link
Collaborator Author

bump, coverage needs to be a bit higher. if you need help seeing where let me know, sometimes the codecov site doesnt show things right away

@coquelin77 Thank you for your offer! Let's see if I can do this, otherwise I'll seek you out.

Copy link
Contributor

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

Looks solid, but it would be good to have at least 1 test for each function where the input tensor is split.

Copy link
Contributor

@TheSlimvReal TheSlimvReal left a comment

Choose a reason for hiding this comment

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

I am happy you could make use of the new testing functions. I just have some recommendations on the usage of it.

self.assertIsInstance(float64_round_distrbd, ht.DNDarray)
self.assertEqual(float64_round_distrbd.dtype, ht.float64)
self.assertEqual(float64_round_distrbd.dtype, ht.float64)
BasicTest.assert_array_equal(self, float64_round_distrbd, comparison)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to extend the BasicTest class and then use self.assert_array_equal. This way self does not need to be passed additionally.

@@ -3,6 +3,8 @@
import numpy as np
import heat as ht

from heat.core.tests.test_suites.basic_test import BasicTest


class TestRounding(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

The BasicTest class is meant as an extension to the unittest.TestCase class and should therefore be extended when you want to use it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for your feedback! I tried to implement your requested changes.

coquelin77
coquelin77 previously approved these changes Dec 6, 2019
Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Well done @lenablind !

@ClaudiaComito ClaudiaComito merged commit 4efe156 into master Dec 9, 2019
@ClaudiaComito ClaudiaComito deleted the features/125-modf branch December 9, 2019 13:19
@lenablind
Copy link
Collaborator Author

Thank you @ClaudiaComito !

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.

Implement modf()
4 participants