modulus with policy and primitive test #692
Conversation
source/framework/algorithm/include/lue/framework/algorithm/modulus.hpp
Outdated
Show resolved
Hide resolved
saeb-faraji-gargari
force-pushed
the
gh667_modulus_v2
branch
from
d56acc1
to
b4afcce
Compare
| @@ -60,6 +60,7 @@ add_library(lue_py_framework SHARED | |||
| src/algorithm/locality_id.cpp | |||
| src/algorithm/log.cpp | |||
| src/algorithm/multiply.cpp | |||
| src/algorithm/modulus.cpp | |||
There was a problem hiding this comment.
Names are sorted by alphabet. Please switch with line above.
There was a problem hiding this comment.
Please move this file into the local_operation directory. Some time ago I have started distributing operations into sub-directories. This is not finished yet, but new algorithms should be put in the right location from the start.
| @@ -0,0 +1,31 @@ | |||
| //#include "lue/framework/algorithm/value_policies/modulus.hpp" | |||
|
|
||
| namespace lue::framework { | ||
|
|
||
| /*template<typename Element, Rank rank, typename Argument1, typename Argument2> |
There was a problem hiding this comment.
Remove commented code. Also, remove comments in bind_modulus.
|
|
||
| { | ||
| // clang-format off | ||
| {908, 12, 200, 5}, |
There was a problem hiding this comment.
Please indent blocks like this. After an opening brace code must be indented.
| auto modulus = array1 % array2; | ||
| lue::test::check_arrays_are_equal(modulus, array_we_want); | ||
| } | ||
|
|
There was a problem hiding this comment.
There should be two blank lines between functions. Clang-format should have fixed this automatically upon committing, like some other formatting errors. Did you install the pre-commit hooks?
| } | ||
|
|
||
| template<typename Element, std::size_t rank> | ||
| void test_modulus_random_array() |
There was a problem hiding this comment.
Please rename. You are not testing random input here.
|
|
||
| Element const nd{lue::policy::no_data_value<Element>}; | ||
|
|
||
| Array<Element, rank> array1 = lue::test::create_partitioned_array<Array<Element, rank>>( |
There was a problem hiding this comment.
Please add the case where the first argument contains data and the second does not.
| } | ||
|
|
||
|
|
||
| // TEST_CASE(1, int32_t) |
There was a problem hiding this comment.
Please remove these comments. I know some other modules also include them. They should all be removed.
| @@ -0,0 +1,219 @@ | |||
| #define BOOST_TEST_MODULE lue framework algorithm multiply | |||
| #include "lue/framework/algorithm/create_partitioned_array.hpp" | |||
| #include "lue/framework/algorithm/default_policies/all.hpp" | |||
There was a problem hiding this comment.
Please remove includes of headers containing code that is not needed here. In general: include the smallest set of header files, containing code that is needed in a module.
| static constexpr bool within_domain( | ||
| [[maybe_unused]] Element const numerator, Element const denominator) noexcept | ||
| { | ||
| return denominator != 0; |
There was a problem hiding this comment.
I think 0 should be changed to Element{0} to prevent compiler warnings. It doesn't hurt anyway.
A literal 0 is of type int which is often int32_t. When Element is an int64_t compilers may start to warn about the difference, or C++ may automatically convert one of the arguments to the type of the other argument. In general, being explicit about these things is good to avoid warnings or confusion.
| { | ||
| public: | ||
|
|
||
| static_assert(std::is_integral_v<InputElement>); |
There was a problem hiding this comment.
I would add an assertion here that OutputElement is the same type as InputElement. The code could actually be simplified to make this obvious everywhere, but that is not necessary now. In general: if there is a very small chance that we ever want to support different types for input and output element types, then we might as well make it impossible to pass in different types.
|
@saeb-faraji-gargari I added minor comments to your code. It starts to look fine. Just a few edits and the PR is good to go. |
saeb-faraji-gargari
force-pushed
the
gh667_modulus_v2
branch
from
b4afcce
to
f69ce39
Compare
No description provided.