cburstedde / libsc

The "sc" auxiliary library

www.p4est.org/

GNU Lesser General Public License v2.1

35 stars 34 forks source link

Feature mpi pack #166

Closed lukasdreyer closed 7 months ago

lukasdreyer commented 7 months ago

MPI Pack/Unpack

Proposed changes: For t8code, we are interested to use MPI_Pack, MPI_Unpack and MPI_Pack_size. This PR introduces a wrapper for these functionalities when libsc is compiled without MPI.

cburstedde commented 7 months ago

Thanks a lot for giving back to sc! Appreciate the update.

Might I ask you to (a) provide a doc/author_dreyer.txt file like the others, (b) provide the usual doxygen comments in the .h and if you have time (c) give the function some minimal test coverage?

lukasdreyer commented 7 months ago

Thanks a lot for giving back to sc! Appreciate the update.

Might I ask you to (a) provide a doc/author_dreyer.txt file like the others, (b) provide the usual doxygen comments in the .h and if you have time (c) give the function some minimal test coverage?

It's a pleasure. Writing a test even helped uncover an error that was still included with multiple elements that were packed.

cburstedde commented 7 months ago

Thanks again! I'm wondering whether you might look at the CI to see what's up.

lukasdreyer commented 7 months ago

Thanks again! I'm wondering whether you might look at the CI to see what's up.

Apparently, I did not end 2 comments correctly, which the compiler had no problems with when compiling without Wall and Werror. Should be fixed now

cburstedde commented 7 months ago

When I run scindent on sc_mpi.c, I get lots of changes of the following form:
-               sc_MPI_Status * status)
+               sc_MPI_Status *status)
Should I include them in this PR as well?

I know, these are annoying. Please ignore those. Thanks!

lukasdreyer commented 7 months ago

When I run scindent on sc_mpi.c, I get lots of changes of the following form: - sc_MPI_Status * status) + sc_MPI_Status *status) Should I include them in this PR as well? I know, these are annoying. Please ignore those. Thanks!

Good to know. Should be ready for the workflow check again

cburstedde commented 7 months ago

When I run scindent on sc_mpi.c, I get lots of changes of the following form: - sc_MPI_Status * status) + sc_MPI_Status *status) Should I include them in this PR as well? I know, these are annoying. Please ignore those. Thanks!

Good to know. Should be ready for the workflow check again

Thanks; I've just pushed some minor convention alignments.

One more thing: The three new functions are currently in a section of the .c file where they are compiled even with MPI enabled. Would you please move them up to the MPI replacement functions that are only compiled without MPI?

cburstedde commented 7 months ago

Thanks again; just left a few messages about the test program. Then ready to merge!

lukasdreyer commented 7 months ago

Thanks again; just left a few messages about the test program. Then ready to merge! Done

cburstedde commented 7 months ago

Thanks your contribution and for following up on the PR!

cburstedde commented 7 months ago

(Sorry misclick. About to merge.)

cburstedde commented 6 months ago

Just checking: how important is MPI_INT8_T for y'all? It's from a relatively new standard version. We'd rather only use MPI up to 1.3. Would you be fine removing that type and also MPI_UNSIGNED_LONG_LONG? Please @hannesbrandt chime in for more details/comments.