-
Notifications
You must be signed in to change notification settings - Fork 62
Add tests for get_chunks and change formula for get_chunks #618
Conversation
|
Hello @PokhodenkoSA! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-02-18 16:01:18 UTC |
sdc/utilities/prange_utils.py
Outdated
|
|
||
| @sdc_register_jitable | ||
| def get_chunks(size, pool_size=0): | ||
| if pool_size == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion - remove this check and always pass pool_size explicitly.
Pure functions are good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now we should explicitly call get_pool_size() and pass it to get_chunks(). Am I correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's not so convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can introduce function like parallel_chunks(N) which will do call get_chunks(N, get_pool_size()). @Hardcode84, @densmirn ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
sdc/tests/test_prange_utils.py
Outdated
| @@ -0,0 +1,82 @@ | |||
| # ***************************************************************************** | |||
| # Copyright (c) 2019, Intel Corporation All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change year in the copyright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Hardcode84
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See tests. They show how it should work.