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

support for bool via kabool wrapper #246

Merged
merged 41 commits into from
Jul 8, 2022
Merged

support for bool via kabool wrapper #246

merged 41 commits into from
Jul 8, 2022

Conversation

niklas-uhl
Copy link
Member

This fixes #219

@niklas-uhl niklas-uhl added the review Ready for review label Apr 10, 2022
Copy link
Member

@kurpicz kurpicz left a comment

Choose a reason for hiding this comment

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

I really like this proposal. However, there are a few things like to be added:

  • Tests for at leas one other MPI operation currently available in KaMPI.ng
  • automatic conversion (with runtime overhead) from std::vector<bool> to std::vector::<kabool>

include/kamping/mpi_datatype.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
@niklas-uhl
Copy link
Member Author

niklas-uhl commented Apr 11, 2022

automatic conversion (with runtime overhead) from std::vector<bool> to std::vector::<kabool>

I don't like automatic conversion from std::vector<bool> to std::vector<kabool>, because it involves copying. With the current interface, it becomes clear to the user that this has a runtime overhead, and the user gets a hint on whether it is preferable to represent the data as std::vector<kabool> throughout the whole application when he has to type:

std::vector<bool> data = {...};
std::vector<kabool> data_to_send {data.begin(), data.end()};

@Hespian
Copy link
Member

Hespian commented Apr 11, 2022

It might be a topic to be discussed with all developers but I agree that automatic conversion would be nice.
At least using send_buf(false) should probably work (not sure if it does right now)

@niklas-uhl
Copy link
Member Author

send_buf(false) is against our current API design, because we don't have single element receive buffers yet and the type of the output buffer is infered from the input type. When I input send_buf(false) the receive buffer is a std::vector<bool> of size 1, which is not compatible with MPI, so we have to also do implicit conversion of the result. That makes it even worse, when we allow passing std::vector<bool> as send_buf and do implicit conversion, we will implicitly copy an arbitrarily sized user input twice. We could mention this in the docs, but then we have weird corner cases, where we incur runtime overhead. From a user perspective its much easier to understand, that you should just not use bool, but wrap it. This is already ergonomic enough because of the implicit conversion bool <-> kabool

@niklas-uhl niklas-uhl requested a review from kurpicz April 13, 2022 13:32
@Hespian Hespian mentioned this pull request Apr 22, 2022
@niklas-uhl niklas-uhl requested review from Hespian and removed request for kurpicz April 25, 2022 12:21
include/kamping/mpi_datatype.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_factories.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_factories.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Outdated Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Outdated Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Outdated Show resolved Hide resolved
tests/parameter_factories_test.cpp Outdated Show resolved Hide resolved
@niklas-uhl niklas-uhl added dependent and removed review Ready for review labels May 12, 2022
@niklas-uhl niklas-uhl added review Ready for review and removed dependent labels Jul 1, 2022
@niklas-uhl niklas-uhl requested a review from Hespian July 1, 2022 14:46
Copy link
Member

@Hespian Hespian left a comment

Choose a reason for hiding this comment

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

Add tests for make_data_buffer and possibly for the buffer factories. I think these are more important than the many tests for some operations that you added. They should just work if the buffer factories work.

tests/collectives/mpi_gather_test.cpp Outdated Show resolved Hide resolved
tests/parameter_objects_test.cpp Outdated Show resolved Hide resolved
tests/vector_bool_compilation_failures_test.cpp Outdated Show resolved Hide resolved
tests/vector_bool_compilation_failures_test.cpp Outdated Show resolved Hide resolved
tests/parameter_objects_test.cpp Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Outdated Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Show resolved Hide resolved
include/kamping/parameter_objects.hpp Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_factories.hpp Outdated Show resolved Hide resolved
@niklas-uhl niklas-uhl requested a review from Hespian July 7, 2022 09:09
Copy link
Member

@Hespian Hespian left a comment

Choose a reason for hiding this comment

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

I know it's annoying, but OwnContainer has do much non-trivial code that I think it should have tests.

include/kamping/parameter_objects.hpp Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
include/kamping/parameter_objects.hpp Outdated Show resolved Hide resolved
tests/collectives/mpi_reduce_test.cpp Outdated Show resolved Hide resolved
tests/helpers_for_testing.hpp Outdated Show resolved Hide resolved
tests/helpers_for_testing.hpp Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@niklas-uhl niklas-uhl requested a review from Hespian July 8, 2022 14:38
@niklas-uhl niklas-uhl merged commit 6d7abc2 into main Jul 8, 2022
@niklas-uhl niklas-uhl deleted the features/bool-vectors branch July 8, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for bool as underlying data type of a buffer.
3 participants