Utilities::pack and unpack #5500
Conversation
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Maybe change the test so that it outputs ok
luca-heltai
force-pushed
the
pack_unpack
branch
from
cdabadc
to
90fe2ca
Compare
include/deal.II/base/utilities.h
Outdated
| * to pack the object into a vector of characters. The object can be unpacked | ||
| * using the Utilities::unpack function below. | ||
| * | ||
| * If the library has been compiled with BZIP enabled, then the output buffer |
There was a problem hiding this comment.
This should probably be ZLIB.
|
/run-tests |
include/deal.II/base/utilities.h
Outdated
| * If the library has been compiled with ZLIB enabled, then the output buffer | ||
| * is compressed. | ||
| * | ||
| * @author Luca Heltai, 2017. |
There was a problem hiding this comment.
since you moved the fucntion, maybe add @tjhei as an author as IIRC he wrote those originally.
There was a problem hiding this comment.
@tjhei wrote the initial function, I added the compression. Either way, though, great idea to externalize this into a separate set of functions!
There was a problem hiding this comment.
great idea to externalize this into a separate set of functions!
👍
There was a problem hiding this comment.
Great idea!
Since it's completely generic now and not replicated in many places, we may later on think about whether the function can be optimized in various ways. For example, if T is either a POD type, or an array of PODs, or a std::vector of POD types, then we can just copy the data into the buffer bitwise. Or not compress if the data size is less than 1000 bytes, or any other considerations. But all of this can wait for later patches and/or evidence that it makes a difference.
In any case, I like this a lot!
include/deal.II/base/utilities.h
Outdated
| * If the library has been compiled with ZLIB enabled, then the output buffer | ||
| * is compressed. | ||
| * | ||
| * @author Luca Heltai, 2017. |
There was a problem hiding this comment.
@tjhei wrote the initial function, I added the compression. Either way, though, great idea to externalize this into a separate set of functions!
include/deal.II/grid/grid_tools.h
Outdated
| } | ||
|
|
||
| return buffer; | ||
| return Utilities::pack(*this); |
There was a problem hiding this comment.
since the function now has only one line, could you just get rid of it altogether by calling Utilities::pack(obj) instead of obj.pack_data()?
There was a problem hiding this comment.
don't we also pack and unpack cell_id within the CellDataTransferBuffer? eventually we should be always sending/receiving those in pairs.
luca-heltai
force-pushed
the
pack_unpack
branch
2 times, most recently
from
a1347ff
to
e4c6905
Compare
| @@ -3290,8 +3216,7 @@ namespace GridTools | |||
| tria->get_communicator(), &status); | |||
| AssertThrowMPI(ierr); | |||
|
|
|||
| CellDataTransferBuffer<dim, DataType> cellinfo; | |||
| cellinfo.unpack_data(receive); | |||
| auto cellinfo = Utilities::unpack<CellDataTransferBuffer<dim, DataType> >(receive); | |||
There was a problem hiding this comment.
out of curiosity, how are std::vector<CellId> cell_ids; (member of CellDataTransferBuffer) being set here?
There was a problem hiding this comment.
p.s. I am just a bit lost how do we unpack this part of the object here... @bangerth ?
There was a problem hiding this comment.
ah, i guess it's hidden in archive << object; and archive >> object; which calls load and save methods of this class and consequently do the right thing.
| namespace serialization | ||
| { | ||
|
|
||
| // Provides boost and c++11 with a way to serialize tuples and pairs automatically |
There was a problem hiding this comment.
do you mind adding tests for those?
There was a problem hiding this comment.
maybe put it into a separate PR so that this one can be merged quickly
There was a problem hiding this comment.
nevermind, i see that you already pushed the tests.
There was a problem hiding this comment.
it's already there. test number two packs and unpack a tuple of pairs and points.
|
@luca-heltai can you please squash into fewer commits, ideally two: first for everything but "Added boost::serialization::serialize for tuples and pairs in utilities" and the second for it. |
luca-heltai
force-pushed
the
pack_unpack
branch
from
e4c6905
to
a741cac
Compare
|
merge? |
Since this is used in many places over the library now, I moved these two to the utilities namespace.
In a followup PR, I'll make sure that whenever this functionality is used in the library, then this is called instead of rewriting the same thing over and over.I already started, by converting some calls insidegrid_tools.h(see last commit).