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

Different dtypes testing #420

Merged
merged 23 commits into from
Dec 6, 2019
Merged

Different dtypes testing #420

merged 23 commits into from
Dec 6, 2019

Conversation

TheSlimvReal
Copy link
Contributor

Description

After I used the TestSuite for the first time in production I found some useful functionalities to extend the module with.

As @coquelin77 requested, the TestSuite is now testing on multiple dtypes which can also be defined by the user.
Internally it used np.random.randn for float values and np.random.randint for integer values.
The latter one also accepts arguments for upper and lower bound of the random values.

Added to that, debugging messages were made more precise about when a exception is risen.

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?
[x] yes [ ] 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?
[ ] yes [x] 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.

just a few minor things, mostly clarifications for my own understanding

heat/core/tests/test_suites/basic_test.py Outdated Show resolved Hide resolved
@@ -209,9 +293,50 @@ def assert_func_equal(
else:
self.assertTrue(np.array_equal(ht_res._DNDarray__array.numpy(), np_res))

def _create_random_array(self, shape):
def _create_random_array(self, shape, dtype=np.float64, low=-10000, high=10000):
Copy link
Member

Choose a reason for hiding this comment

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

can you note that this creates a numpy array? others might misjudge it by its name, maybe just change array to np_array? just a thought

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name to __create_random_np_array

@coquelin77
Copy link
Member

also can you trigger a new coverage run?

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #420 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage      98%   98.02%   +0.01%     
==========================================
  Files          55       55              
  Lines       11041    11066      +25     
==========================================
+ Hits        10821    10847      +26     
+ Misses        220      219       -1
Impacted Files Coverage Δ
heat/core/tests/test_suites/test_basic_test.py 100% <100%> (ø) ⬆️
heat/core/tests/test_suites/basic_test.py 100% <100%> (+1.47%) ⬆️

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 25fdd0b...b0a395f. Read the comment docs.

@TheSlimvReal
Copy link
Contributor Author

I removed the communicator tests in favour of #422. Once this discussion comes to a conclusion, the comm test might be added again.

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.

this all looks great to me. maybe a future addition as well would be an option to suppress the prints for travis.

@coquelin77 coquelin77 merged commit 3e7c397 into master Dec 6, 2019
@coquelin77 coquelin77 deleted the different_dtypes_testing branch December 6, 2019 14:50
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.

2 participants