implement additional unit tests for communication pattern #143
Conversation
|
The The expected size should be Run the unit test |
| auto comm = exec->get_gko_mpi_host_comm(); | ||
| EXPECT_EQ(comm->size(), 4); |
There was a problem hiding this comment.
we still need a test if comm->size() works and returns a result that is > 1
There was a problem hiding this comment.
Okay, but it is better to put this test outside of any specific unit test.
All the unit tests should not be run if the size of a communicator is not great than 1. For example, it should be checked during the construction of the fixture object.
There was a problem hiding this comment.
All functionalities of a communicator should be verified in the corresponding unit tests for communicator.
There was a problem hiding this comment.
Ok I am fine with either approach, the main point is that it should be directly obvious that the test should fail if it is not run with sufficient number of ranks!
unitTests/CommunicationPattern.C
Outdated
| for (int i = 0; i < comm_size; i++) | ||
| send_counts[i] = num_elements*i; |
There was a problem hiding this comment.
Please add curly braces here, even if it is correct notation without.
| for (int i = 0; i < comm_size; i++) | |
| send_counts[i] = num_elements*i; | |
| for (int i = 0; i < comm_size; i++) { | |
| send_counts[i] = num_elements*i; | |
| } |
unitTests/CommunicationPattern.C
Outdated
| // Scatter different number of elements from owner to each non-owner process | ||
| std::vector<int> send_counts(comm_size); | ||
|
|
||
| if (comm_rank == 0) |
There was a problem hiding this comment.
Please add curly braces here
to make it clear that there is only one owner process in the test
The unit test does not verify any functionality of CommunicationPattern. It verifies the functionality of a MPI communicator, and thus should not belong to the test suite for CommunicationPattern. To make some hardcoded unit tests more self-contained, the ascertain part for the communicator size is added accordinlgy.
of two owner processes
All processes in the communicator are owner processes.
…Pattern to make the test code more readable
…rocess to improve readability
for the case of only a single owner process so that any number of elements can be set for the test
In the arrange section, 1. remove the vector of vector previously used for expected results Use a vector for the expected result instead. 2. Add vectors for the expected results of receive parts (recv_counts and recv_offsets) In the assert section, use vectors instead of vectors of vectors for both send and receive parts
1. Use vectors instead of vectors of vectors for expected results. 2. Parameterize the communicator size to avoid the repeated function calls.
… that it is not hardcoded anymore. Now it can run on an arbitrarily even number of processes.
in the increasing number of owner processes.
for the case of two owner processes
and "compute_gather_to_owner_counts_single_owner" to verify that each process can send a different number of elements to owner processes.
for the moment. Modify the unit test "compute_scatter_from_owner_counts_single_owner" so that the owner can send data to itself.
If only 1 process is used, the program will abort.
…e structures more easily identified.
…ts" for the case of two owners.
greole
force-pushed
the
impl/unit_tests_communication-pattern
branch
from
95aef04
to
6d09fb2
Compare
greole
changed the title
Add new unit tests for the following functions
compute_owner_rankcompute_gather_to_owner_countscompute_scatter_from_owner_countsRefactor original unit tests for the function
compute_gather_to_owner_countsThe unit tests can run on any arbitrary even number of CPU processes.