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
Unified interface for serializing triangulations. #15576
Conversation
9c54453
to
cb56506
Compare
@marcfehling @blaisb @peterrum @bangerth anybody willing to review this? The unchecked listed items will be addressed in subsequent PRs. |
@pcafrica I will review it first thing in the morning :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tiny comment. Nice changes really. I did not find anything "wrong" that caught my eyes. The only worry I would have would be that this will break all user code that use the cell status from the triangulations (including ASPECT). I don't know if there could be a provision to keep the cell status defined within the triangulation as an alias?
Thanks for pointing out, my last commit should address this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good 👍
8d3ba16
to
9ff1b0c
Compare
Thank you @peterrum! The last 4 commits should do it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the rest of my comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, getting very close!
Once you make your last changes, you might also want to squash commits into a smaller number. |
a751b51
to
8a231a8
Compare
9594a99
to
3de236e
Compare
3de236e
to
6bd7601
Compare
6bd7601
to
f765e89
Compare
Heads up. This pull request should create some noticeable fallout on the regression tester. It breaks at least our performance tests running on bullseye. |
This broke ASPECT and will likely break other codes. See for example geodynamics/aspect#5251, and error messages are of the following form:
(Taken from https://github.com/geodynamics/aspect/actions/runs/5511284508/jobs/10046617673?pr=3773 for geodynamics/aspect#3773 .) Does anyone know why the attempted backward compatibility doesn't work here? We should really push a follow-up that restores backward compatibility. |
mhm I fully agree with @bangerth here.
This kind of backwards compatibility should not be too costly. |
A The reason is still unknown to me, I'm currently trying to produce a MWE that replicates the issue. |
@pcafrica Yes, I realize. I am investigating as well. Try compiling |
@pcafrica One problem I see is that both objects are not the same. The compiler has no way of converting a |
Another subtle point is the difference that an |
Unfortunately, using enums was not allowed before C++20: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html |
That's not quite true. You cannot import them but still alias, see https://godbolt.org/z/4d5EejEaz. |
@masterleinad @pcafrica Let's see what the CI is saying wrt #15707 |
I might have found the non-backward compatible lines, where the new By the way, aliasing looks promising and a better solution. Thanks for the fix! |
This PR aims at providing a uniform interface for serializing/deserializing all different triangulation classes.
Before this PR, the most abstract interface was available in the
parallel::DistributedTriangulationBase
class. In order to abstract it toTriangulation
, the following steps were/are required:CellAttachedData
fromP::DTB
togrid/tria.h
(internal
namespace).DataTransfer
fromP::DTB
togrid/tria.h
(internal
namespace) and renamed toCellAttachedDataSerializer
. This class currently is hard-coded to use MPI:DataTransfer
depended on aliases and members defined in theTriangulation
base class, in particularCellStatus
, which has been moved to a separategrid/cell_status.h
file.The goal is to use a new hierarchy of classes polymorphically, so thatT
stores a pointer to an abstractDataTransfer
interface.Two classes(not possible sinceDataTransferSerial
andDataTransferParallel
will inherit fromDataTransfer
(still to implement in the newgrid/data_transfer.h
file).DataTransfer
is templatized).T
andp::DTB
will take care of initializingDataTransfer
.DataTransfer
for the case MPI support is disabled.save(std::string filename)
andload(std::string filename)
fromp::DTB
toT
, possibly avoiding clashes with the Boost serialization methods (template <Archive ar> save/load(ar, version)
). (Done in Move the save and load function to serial tria #15615)DataTransfer
: it does not store anymore an MPI communicator as a member, because being it constructed now fromT
, that would always beMPI_COMM_SELF
. Rather, the MPI communicator is now taken as an input by all theDataTransfer
methods that needs it.@blaisb @peterrum @bangerth