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
Add inter-grid transfer operators #10417
Conversation
451e863
to
e16d7a8
Compare
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.
Would you mind describing the new classes on a high level? I got lost when looking at the review.
I left some initial comments.
|
||
|
||
/** | ||
* Class for repartitioning data living on the same triangulation, which |
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.
triangulation or dofhandler? Or is this more generic? Are these vectors the same size?
This is probably unrelated to multigrid and could go somewhere else (vectortools?).
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.
This is probably unrelated to multigrid and could go somewhere else (vectortools?).
But for now I would like to keep in this PR since it uses the same infrastructure as the global-coarsening algorithm is using.
@tjhei Sorry for the late reply! The idea is following. Let's consider for now two levels (the level with more dofs I will call fine level; the other one is the coarse level). The class
This information is encrypted in the class The class In the namespace |
* elements are currently not implemented. | ||
*/ | ||
template <typename MatrixType> | ||
class MGTransferMatrixFreeNew |
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.
maybe MGTransferGlobalCoarsening?
Maybe move VectorRepartitioner into separate PR? |
4f7b7e9
to
974ad4f
Compare
Done!
I have removed it. Once this PR is merged, I will open the PR for that.
Could you have a look at the utility functions. There users have the possibility to pass constraint matrices. In what way are these different from custom constraints? |
/rebuild |
974ad4f
to
c9eacda
Compare
* MGTransferUtilities to setup the Transfer operators. | ||
*/ | ||
template <typename Number> | ||
struct MGTransferScheme |
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.
If I understand it correctly, then this is an implementation detail of TwoLevelTransfer, right? I would make this a member of it or move it into an implementation namespace.
* Class for transfer between two multigrid levels. | ||
*/ | ||
template <int dim, typename Number> | ||
class MGTwoLevelTransfer |
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.
I would suggest templating on VectorType and only implementing d:Vector for now.
|
||
|
||
/** | ||
* Class for transfer between two multigrid levels. |
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.
can you extend to say that this can be p or global coarsening?
* elements are currently not implemented. | ||
*/ | ||
template <typename MatrixType> | ||
class MGTransferMatrixFreeNew |
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.
consider renaming to GlobalCoarsening
: matrices(matrices) | ||
, transfer(transfer) | ||
{ | ||
AssertDimension(matrices.max_level() - matrices.min_level(), |
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.
the documentation of this function talks about l and l-1 entries, but here you assert that they are equal. Fix, please.
bool | ||
run(Fu &fu) | ||
{ | ||
return do_run_degree<1>(fu) || do_run_degree<2>(fu) || |
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.
what about degree=0?
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.
We can think about degree=0
. But let's leave that for a follow-up PR.
* 2) bisect, and 3) decrease-by-one is supported. | ||
*/ | ||
bool | ||
polynomial_transfer_supported(const unsigned int fe_degree_fine, |
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.
is this just FE_Q or more generic?
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.
Currently FE_Q
and FE_DGQ
(and I guess FE_DGQArbitraryNodes
). I guess it should be the same as in the case MGTransferMatrixFree
.
@tjhei I have made the changes. |
|
||
|
||
/** | ||
* Implementation of the MGTransferBase interface for which the transfer |
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.
this documentation is outdated. Please mention p and h global coarsening.
|
||
DEAL_II_NAMESPACE_OPEN | ||
|
||
namespace MGTransferUtilities |
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.
Please document the namespace. Also, the name is not ideal, is it?
* 2) bisect, and 3) decrease-by-one is supported. | ||
*/ | ||
bool | ||
polynomial_transfer_supported(const unsigned int fe_degree_fine, |
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.
Based on the documentation, I have no idea what this function does. Would this be more appropriate as a static member of MGTransferTwoLevel? I assume that this is what the function reports on.
*/ | ||
template <int dim, typename Number, typename MeshType> | ||
void | ||
setup_global_coarsening_transfer( |
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.
Make this a constructor of MGTwoLevelTransfer?
*/ | ||
template <int dim, typename Number, typename MeshType> | ||
void | ||
setup_polynomial_transfer( |
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.
Same.
MGTwoLevelTransfer<dim, LinearAlgebra::distributed::Vector<Number>> | ||
&transfer); | ||
|
||
} // namespace MGTransferUtilities |
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.
With all three functions gone, we could get rid of this namespace and file.
@tjhei I have following changes:
Le's not over-think the structure of the internal functions. My guess is once we want to support additional vector types, we can reuse many functions. |
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.
Thanks for the quicj edits. Things look much better now. I have more complaints though, sorry.
* scheme for transfer between children and parent cells, as well as, one | ||
* transfer scheme for cells that are not refined). | ||
*/ | ||
struct MGTransferScheme |
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.
does this need to be public?
// --------------------------------------------------------------------- | ||
|
||
|
||
// Test ConsensusAlgorithms::AnonymousProcess. |
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.
update
|
||
|
||
/** | ||
* |
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.
"Test MGTwoLevelTransfer with mesh coarsening"
deallog.precision(8); | ||
|
||
for (unsigned int i = 1; i < 5; i++) | ||
test<2, double>(i); |
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.
The test output seems to be really large (github doesn't even show it). Can you see if this can be reduced somewhat?
|
||
|
||
/** | ||
* |
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.
Test MGTwoLevelTransfer with polynomial coarsening
fe_degree_coarse++) | ||
if (MGTwoLevelTransfer<2, LinearAlgebra::distributed::Vector<double>>:: | ||
polynomial_transfer_supported(fe_degree_fine, fe_degree_coarse)) | ||
test<2, double>(fe_degree_fine, fe_degree_coarse); |
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.
same here, the files seem to be quite big?
|
||
|
||
/** | ||
* |
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.
same here, add a one line description
DEAL::1 1 1 1 | ||
DEAL::1 1 1 1 | ||
DEAL::1 1 1 1 | ||
DEAL::1 1 1 1 |
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.
beautiful!
tests/multigrid-transfer/test.h
Outdated
@@ -0,0 +1,167 @@ | |||
// --------------------------------------------------------------------- |
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.
I just realized that this is a new folder. Don't you need to add a CMakeList.txt? Also, maybe we should name this folder multigrid-global-coarsening ?
@tjhei I have addressed your comments. Once you are happy, I will squash. |
Looks good, please squash. |
e92ef3d
to
bf26fad
Compare
@tjhei Done! |
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.
This looks good to me, but I will let somebody else to take a look. @kronbichler ?
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.
Apart from a few comments, this looks good to me.
return do_run_degree<1>(fu) || do_run_degree<2>(fu) || | ||
do_run_degree<3>(fu) || do_run_degree<4>(fu) || | ||
do_run_degree<5>(fu) || do_run_degree<6>(fu) || | ||
do_run_degree<7>(fu) || do_run_degree<8>(fu) || | ||
do_run_degree<9>(fu); |
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.
I know that other cases are supposed to happen later; my question would be if that follow-up code would then go to a generic version with -1
template argument (or 0
in the tensor product evaluators) so we do not bail out.
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.
I see your point. Let's leave it to a follow-up PR.
include/deal.II/multigrid/mg_transfer_global_coarsening.templates.h
Outdated
Show resolved
Hide resolved
AssertDimension(dof_handler_fine.get_fe(0).n_base_elements(), 1); | ||
std::string fe_name = | ||
dof_handler_fine.get_fe(0).base_element(0).get_name(); | ||
{ | ||
const std::size_t template_starts = fe_name.find_first_of('<'); | ||
Assert(fe_name[template_starts + 1] == | ||
(dim == 1 ? '1' : (dim == 2 ? '2' : '3')), | ||
ExcInternalError()); | ||
fe_name[template_starts + 1] = '1'; | ||
} | ||
const std::unique_ptr<FiniteElement<1>> fe( | ||
FETools::get_fe_by_name<1, 1>(fe_name)); | ||
|
||
transfer.schemes[0].prolongation_matrix_1d.resize(fe->dofs_per_cell * | ||
fe->dofs_per_cell); | ||
|
||
for (unsigned int i = 0; i < fe->dofs_per_cell; i++) | ||
transfer.schemes[0] | ||
.prolongation_matrix_1d[i + i * fe->dofs_per_cell] = Number(1.0); |
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.
This is a candidate for #9544.
const auto ierr = cell_transfer.run(cell_prolongator); | ||
(void)ierr; | ||
Assert(ierr, | ||
ExcMessage("Prolongation " + | ||
std::to_string(scheme.degree_coarse) + " -> " + | ||
std::to_string(scheme.degree_fine) + | ||
" not instantiated!")); |
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.
This is the place I was referring to in my comment above about the default case degree=-1
.
bf26fad
to
7e4f149
Compare
7e4f149
to
5118bbc
Compare
@kronbichler I have addressed your comments! |
No description provided.