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

Naming of MultigridVariant::LocalSmoothing/GlobalCoarsening #445

Closed
nfehn opened this issue May 23, 2023 · 11 comments · Fixed by #490
Closed

Naming of MultigridVariant::LocalSmoothing/GlobalCoarsening #445

nfehn opened this issue May 23, 2023 · 11 comments · Fixed by #490
Labels
matrix-free Matrix-free implementation multigrid Multigrid implementation

Comments

@nfehn
Copy link
Member

nfehn commented May 23, 2023

@peterrum mentioned that the naming LocalSmoothing/GlobalCoarsening is not really suitable, since we always use kind of global-coarsening multigrid.

I suggest the following renaming:

Old

enum class MultigridVariant
{
  LocalSmoothing,
  GlobalCoarsening
};

New

enum class GeometricMultigridTransferType
{
  HypercubeNoHangingNodes, // Optimized implementation for tensor-product elements and meshes without hanging nodes.
                        // The name reflects that this option is not available for hanging nodes or 
                        // simplex elements (e.g. due to the limitations of dealii::MGTransferMatrixFree or 
                        // missing hanging-node matrix-free multigrid implementations in ExaDG).
  GlobalCoarsening, // the new "default" multigrid transfer type, potentially slower than the first variant
  NonMatching // to be implemented in the future :)
};

In my opinion, usability and the application perspective is particularly important here; e.g. it does not help if we call the first variant MGTransferMatrixFree, since this name does not reflect the functionality/limitations.

Please share your opinion, e.g. @peterrum @kronbichler @bergbauer.

@nfehn nfehn added multigrid Multigrid implementation matrix-free Matrix-free implementation labels May 23, 2023
@kronbichler
Copy link
Contributor

I agree with these names, they better reflect the functionality. I also think that the GlobalCoarsening part must not be much slower, even though it is too slow to set up (but it runs reasonably well).

I've been working towards some optimizations in deal.II recently, and I've remarked in deal.II how to address the most pressing issue: dealii/dealii#14968 - in general, my opinion is that it should be possible to construct these transfer objects from MatrixFree objects and re-use all functionality there. Not sure if all cases can be run through those interfaces, but it should be the goal to avoid touching the DoF indices a second time at the very least, and use better data structures for the identification in the setup. In the end, I believe that the fully flexible geometric transfer will most often not run on the finest problem, so as long as it scales it should not be a bottleneck.

@bergbauer
Copy link
Member

I think it is a bit odd that one name describes the limitations of the underlying algorithms and the other two describe the underlying algorithms. The comments are definitely helpful and I also like the new enum class name much better. Maybe also the possibilities and limitations of GloabalCoarsening could be mentioned here?

@kronbichler
Copy link
Contributor

@bergbauer would you be happier with HypercubesUniformlyRefinedMesh as the name? Also NonMatching and GlobalCoarsening are also different categories, because NonMatching also belongs to the class of global coarsening methods. But I am not sure if I like GlobalCoarseningNested and GlobalCoarseningNonNested better than the other names. In the end, the difficulty is to express different categories in compact names, so I guess the comment next to it are also important.

@bergbauer
Copy link
Member

would you be happier with HypercubesUniformlyRefinedMesh as the name?

No not really, personally I would keep LocalSmoothing but I am fine if you want to change the name to something more descriptive.

Also NonMatching and GlobalCoarsening are also different categories, because NonMatching also belongs to the class of global coarsening methods. But I am not sure if I like GlobalCoarseningNested and GlobalCoarseningNonNested better than the other names. In the end, the difficulty is to express different categories in compact names, so I guess the comment next to it are also important.

Sounds to me as this would rather be a new enum GlobalCoarseningType::Nested/NonNested.

I think the proposed solution is fine with appropriate comments for every GeometricMultigridTransferType. 👍🏽

@nfehn
Copy link
Member Author

nfehn commented May 24, 2023

I think HypercubesUniformlyRefinedMesh and HypercubeNoHangingNodes is not the same.

Question: Would a coarse grid with hanging nodes, subsequently refined uniformly, work with our old "local smoothing" infrastructure?

  • if yes, the name HypercubesUniformlyRefinedMesh would be better
  • if no, the name HypercubeNoHangingNodes would be better

The name LocalSmoothing is inappropriate because smoothing only on parts of the domain is not possible with our implementation in ExaDG for any of the multigrid transfer variants.

I do not see another type GlobalCoarseningType, because our current LocalSmoothing implementation is also "global coarsening", which is the main motivation for this issue. Of course, this raises the question whether the second option GlobalCoarsening is a good name. However, it seems as if we do not easily find better suggestions so far.

@kronbichler
Copy link
Contributor

Question: Would a coarse grid with hanging nodes, subsequently refined uniformly, work with our old "local smoothing" infrastructure?

A coarse grid in deal.II cannot have hanging nodes, so any mesh that has hanging nodes, will not be uniformly refined. All hanging node algorithms look at the level between cells to construct interpolation matrices, so if there should be any refinement, a hanging node can only appear if there is non-uniform refinement somewhere in the mesh creation step.

I agree that the name LocalSmoothing should be avoided. Apart from that, I have no strong opinion on whether to use HypercubesUniformlyRefinedMesh or HypercubeNoHangingNodes - I was suggesting the former because the notion of what is a hanging node is in my opinion more abstract than what a uniform refinement is (and because it says what it needs, not what it does not support). If we want to stress that all algorithms are of GlobalCoarsening type, we might probably want to have names like UniformRefinement/NoHangingNodes -> GenericNested -> NonNested. These names are up to discussion, my intent was to skip GlobalCoarsening from the name, apart from saying that MultigridVariant selects which type of global coarsening algorithm we want to have.

@nfehn
Copy link
Member Author

nfehn commented May 24, 2023

Regarding UniformRefinement: From a naive user perspective, ond could expect to be able to locally refine a deal.II-coarse-grid to obtain a ExaDG-multigrid-coarse-grid e.g. used for the AMG coarse grid solver, and expect the transfer implementation in deal.II behind the current variant LocalSmoothing to be able to deal with this. For this reason, the name GeometricMultigridTransferType::UniformRefinement might be misleading in my opinion. Put differently, I believe it requires knowledge about dealii that I do not want to expect from ExaDG users. Vielleicht reden wir aber auch gerade aneinander vorbei.

From the two naming variants

  • UniformRefinement/NoHangingNodes and GenericNested
  • HypercubesUniformRefinement/HypercubesNoHangingNodes and Nested

I would prefer the second one, because it is more explicit about the limitations of the first variant (where we would remove Hypercubes from the name as soon as this limitation is removed). I interpret "generic = support for hanging nodes / simplex" (?), but the combination of both features is again not supported. This could be a question that comes up when deciding for names with "Generic".

@kronbichler
Copy link
Contributor

Yes, the second variant sounds good to me. Also for the first one, there is no fundamental disagreement, I wanted to raise the question of what is more intuitive, and in that sense I am fine with HypercubesNoHangingNodes (in I forgot the former name part in the latter part of my message). So to me this is good, what about @bergbauer?

@nfehn
Copy link
Member Author

nfehn commented May 24, 2023

I have just seen in the multigrid implementation the code

  else if(multigrid_variant == MultigridVariant::LocalSmoothing)
  {
    AssertThrow(grid->triangulation->has_hanging_nodes() == false,
                dealii::ExcMessage("Hanging nodes are only supported with the option "
                                   "use_global_coarsening enabled."));
    AssertThrow(grid->triangulation->all_reference_cells_are_hyper_cube(),
                dealii::ExcMessage("This multigrid implementation is currently only available for "
                                   "hyper-cube elements. Other grids need to enable the option "
                                   "use_global_coarsening."));
    ...
  }

This nicely demonstrates what the name LocalSmoothing actually means, but this info is currently somewhat hidden for the user.

@nfehn
Copy link
Member Author

nfehn commented May 24, 2023

We also need to think about how to do the renaming in functions like this one:

get_mesh_smoothing(bool const use_local_smoothing_multigrid, ElementType const & element_type)
{
typename dealii::Triangulation<dim>::MeshSmoothing mesh_smoothing;
if(element_type == ElementType::Simplex)
{
mesh_smoothing = dealii::Triangulation<dim>::none;
// the option limit_level_difference_at_vertices (required for local smoothing multigrid) is not
// implemented for simplicial elements.
}
else if(element_type == ElementType::Hypercube)
{
if(use_local_smoothing_multigrid)
mesh_smoothing = dealii::Triangulation<dim>::limit_level_difference_at_vertices;
else
mesh_smoothing = dealii::Triangulation<dim>::none;
}
else
{
AssertThrow(false, dealii::ExcMessage("Not implemented."));
}
return mesh_smoothing;
}

use_local_smoothing_multigrid -> multigrid_with_only_one_triangulation?

@nfehn
Copy link
Member Author

nfehn commented Jun 9, 2023

This issue becomes relevant again once #487 is done (or during a PR resolving #487). Until then, it probably does not make much sense to discuss namings theoretically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matrix-free Matrix-free implementation multigrid Multigrid implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants