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

Flatten dofmap storage #2632

Merged
merged 28 commits into from
Apr 22, 2023
Merged

Flatten dofmap storage #2632

merged 28 commits into from
Apr 22, 2023

Conversation

garth-wells
Copy link
Member

This change stores dofmap data as flat vectors rather than AdjacencyLists, and uses mdspan for convenient access. This uses less memory that AdjacencyLists and data access is more performance friendly.

Assumption is that all elements in a DofMap have the same number io dofs. This is presently that case, and for more complex cases in the future, e.g. mixed cell types, each element type will have its own DofMap. This follows the design that has been started for mixed topology, where cells are stored by type, and operations like assembly will be per cell type/kernel.

It's useful to make this change now as unpicking the code will be a lot harder once mixed topology support is further developed. Moreover, removing necessary use of AdjacencyList makes considerations dealing with #2624 simpler as the impact of any change is reduced.

@garth-wells garth-wells added the enhancement New feature or request label Apr 20, 2023
Copy link
Sponsor Member

@jorgensd jorgensd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general Im in favor of this change, as it would simplify libraries such as dolfinx_mpc and asimov_contact.
See comments for minor things.

cpp/dolfinx/fem/DofMap.h Show resolved Hide resolved
cpp/dolfinx/fem/DofMap.cpp Show resolved Hide resolved
cpp/dolfinx/fem/DofMap.cpp Show resolved Hide resolved
cpp/dolfinx/fem/dofmapbuilder.cpp Show resolved Hide resolved
cpp/dolfinx/io/vtk_utils.cpp Show resolved Hide resolved
cpp/dolfinx/refinement/plaza.h Outdated Show resolved Hide resolved
python/dolfinx/wrappers/fem.cpp Show resolved Hide resolved
python/dolfinx/wrappers/mesh.cpp Outdated Show resolved Hide resolved
python/test/unit/fem/test_assembler.py Outdated Show resolved Hide resolved
@francesco-ballarin
Copy link
Member

This change will make it a bit more difficult for me in multiphenicsx, but I'll definitely take some time next week to sort things out on my end. It was beneficial for me to have

  const graph::AdjacencyList<std::int32_t>& list() const;

in my dofmaps since I cannot assume that every cell has the same number of dofs. To help me sort this out, is there any way I can create a graph::AdjacencyList<std::int32_t> from a std::experimental::mdspan<const std::int32_t, std::experimental::dextents<std::size_t, 2>>, without making any copies?

@garth-wells
Copy link
Member Author

This change will make it a bit more difficult for me in multiphenicsx, but I'll definitely take some time next week to sort things out on my end. It was beneficial for me to have

  const graph::AdjacencyList<std::int32_t>& list() const;

in my dofmaps since I cannot assume that every cell has the same number of dofs.

The approach to this that we're following is to have separate DofsMap for each 'element' type. But we're not quite there yet.

To help me sort this out, is there any way I can create a graph::AdjacencyList<std::int32_t> from a std::experimental::mdspan<const std::int32_t, std::experimental::dextents<std::size_t, 2>>, without making any copies?

No, because graph::AdjacencyList<std::int32_t> owns its data. Make a copy should be pretty cheap, however.

@francesco-ballarin do you use the DOLFINx assemblers in multiphenicsx, or have you written your own?

@garth-wells garth-wells requested a review from jpdean April 21, 2023 08:30
@francesco-ballarin
Copy link
Member

@garth-wells I am reusing as much as I can from DOLFINx assembly, with a few minimal changes in matrix/vector allocation that I keep in sync with the upstream repo. The first place in which I swap out a standard dolfinx dofmap for a graph::AdjacencyList<std::int32_t> is
main...francesco-ballarin:dolfinx:pr1
with the goal of allowing to use my own dofmap (which does not inherit from dolfinx one).

I guess I can still swap that out for a pair containing: as first item, the std::vector of the flattened dof map, and as second item a vector of offsets.
In my implementation I basically have them already
https://github.com/multiphenics/multiphenicsx/blob/main/multiphenicsx/cpp/multiphenicsx/fem/DofMapRestriction.cpp#L258
and I just need to store them (rather than providing them to an adjacency list). Still, since I want my assembly functions to be working also with the standard dolfinx dofmap (rather than my own), I will try to work out a way on how to get that same pair from the new DofMap::list() method, where the first item of the pair should basically be the underlying std::vector which is stored as private member DofMap::_dofmap, and the second item of the pair a std::iota built from DofMap::_shape1, i.e. the second extent of DofMap::list()

@garth-wells garth-wells added this pull request to the merge queue Apr 22, 2023
Merged via the queue into main with commit f6c2921 Apr 22, 2023
15 checks passed
@garth-wells garth-wells deleted the garth/flatten-dofmap branch April 22, 2023 18:00
@francesco-ballarin
Copy link
Member

I guess I can still swap that out for a pair containing: as first item, the std::vector of the flattened dof map, and as second item a vector of offsets.

I confirm I have proceeded in this way, and restored multiphenicsx:main compatibility with dolfinx:main 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants