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

Introduce DoFHandlerBase #9760

Closed
wants to merge 1 commit into from
Closed

Conversation

peterrum
Copy link
Member

I will be working in close collaboration with @kronbichler on the evergreen issue #2000. In that issue many topics have been discussed, most notably, the refactoring of internal data structures of the DoFHandler. I would like to propose to replace the current implementation by a flat CRS-based implementation (suited both for the regular and the hp case).

As a first step, this PR merges the implementation of DoFHandler and hp::DoFHandler in a new class DoFHandlerBase. I regard this as a first step towards a possible Native support for the hp case (see the discussion in #2000). However, the intermediate goal of this change is that improvements to the DoFHandler automatically are part also hp::DoFHandler.

The consequence of rewriting the internal data structures is that also parts of the DoFAccessors classes (these are the classes directly accessing the internal data structures of the DoFHandler) have to be rewritten. However, the consequence might be that no different specializations for DoFHandler and hp::DoFHandler is not needed any more (at least in the case of the accessors). Once this is done, the DoFHandlerBase could be renamed to DoFHandler (and hp::DoFHandler can be removed since all its functionalities are incorporated in DoFHandler).

Now to the details of this PR:

  • it introduces the class DoFHandlerBase (containing the implementations from DoFHandler, hp::DoFHandler)
  • the classes DoFHandler and hp::DoFHandler are hollow implementations to be compatible with the rest of the code and the iterators
  • the purpose of DoFHandlerBase (to work in hp-context or not) is determined via a template parameter
  • many functions and internal variables could be reused for both contexts (which is the main reason for being able to remove about 1500 locs).

I would like to point out the internal data structures:

  • vertex_dofs, levels, faces for normal operation
  • vertex_dofs, vertex_dof_offsets, levels_hp, faces_hp for hp
  • mg_vertex_dofs, mg_levels, mg_faces for multigrid

If the direction of this PR is accepted, I will be working (in a follow-up PR) on merging the first two sets of data structures.

The multgrid data could be adjusted accordingly, so that - if the right transfer operator given - geometric multigrid might work also in the hp case.

You will probably see that in the new code there are the following sins:

  • if (is_hp_dof_handler)
  • the DoFHandlerBase has a third template parameter
  • there are new (dummy) specializations for DoFHandler and hp::DoFHandler

Point 2 and 3 should be resolved once the internal data structures of DoFHandler and hp::DoFHandler are merged and no specializations (e.g. in DoFAccessor) are needed.

Once, we agree that this is a useful change, I would try to implement this as soon as possible (next two weeks), so my sins are not part of the release.

Regarding incompatibility. At the moment, I don't think there will be any major incompatibility issues (if any). The DoFHandlerBase is at the moment a union of the methods of DoFHandler and hp::DoFHandler (e.g. it takes FiniteElement and FECollection). Once DoFHandlerBase is renamed to DoFHandler, the hp::DoFHandler could inherit from DoFHandler and could be deprecated.

Does anyone see any further issues I was not thinking about?

This PR does not contain any changes related to the functionalities and the internal data structures. It copies the content of the DoFHandler- and hp::DoFHandler-related files to new files (and removes some duplicates). So that the 6000 lines of addition are not actually 6000 new lines of code.

The variable is_hp_dof_handler might become in the future to a synonym of fe_collection.size()>1. Depending on this, different policies could be uses. I.e. we will be able to switch dynamically policies. But to implement this is not urgent to the release.

All tests ctest -j30 -R "hp|dofs|matrix_free" have passed. I am currently running the full test suite.

@kronbichler
Copy link
Member

/rebuild

@peterrum peterrum force-pushed the dofhandler branch 2 times, most recently from 9be2778 to e6b6375 Compare March 28, 2020 17:17
@peterrum
Copy link
Member Author

I have run the test suite with the following configuration:

/lnm/bin/cmake \
    -D CMAKE_CXX_COMPILER="mpicxx" \
    -D CMAKE_C_COMPILER="mpicc" \
    -D CMAKE_BUILD_TYPE="Debug" \
    -D CMAKE_CXX_FLAGS="-march=skylake-avx512 -pthread -fopenmp" \
    -D DEAL_II_LINKER_FLAGS="-fopenmp" \
    -D CMAKE_C_FLAGS="-march=skylake-avx512" \
    -D DEAL_II_CXX_FLAGS_RELEASE="-O3" \
    -D DEAL_II_WITH_MPI:BOOL="ON" \
    -D DEAL_II_WITH_TRILINOS:BOOL="ON" \
    -D TRILINOS_DIR="..." \
    -D DEAL_II_WITH_PETSC:BOOL="ON" \
    -D PETSC_DIR="..." \
    -D PETSC_ARCH="arch-linux2-c-opt" \
    -D DEAL_II_WITH_METIS:BOOL="ON" \
    -D MPIEXEC_PREFLAGS="-bind-to none" \
    -D METIS_DIR="..." \
    -D DEAL_II_WITH_THREADS:BOOL="ON" \
    -D DEAL_II_WITH_NETCDF:BOOL="OFF" \
    -D DEAL_II_FORCE_BUNDLED_BOOST="ON" \
    -D DEAL_II_UNITY_BUILD="ON" \
    -D DEAL_II_WITH_P4EST="ON" \
    -D P4EST_DIR="..." \
    -D DEAL_II_COMPONENT_DOCUMENTATION="OFF" \
    -D DEAL_II_STATIC_EXECUTABLE="OFF" \
    -D LAPACK_LIBRARIES="..." \
    ../dealii

with the following result:

99% tests passed, 8 tests failed out of 5734

Total Test time (real) = 3818.92 sec

The following tests FAILED:
	1285 - bits/step-51p.debug (Failed)
	1491 - distributed_grids/3d_coarse_grid_02.debug (Failed)
	2501 - grid/grid_out_in_vtk_01.debug (Failed)
	2510 - grid/grid_out_vtk_03.debug (Failed)
	3986 - mpi/multigrid_adaptive.mpirun=1.debug (Failed)
	3987 - mpi/multigrid_adaptive.mpirun=4.debug (Failed)
	3988 - mpi/multigrid_uniform.mpirun=1.debug (Failed)
	3989 - mpi/multigrid_uniform.mpirun=4.debug (Failed)

This is identical to the test run with dealii/master.

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

I suspect nobody is excited about reviewing this :-( Do you think you could chop off smaller pieces that could be independently merged? For example, the block_info.* changes seem relatively unrelated. There are also a lot of additions of dealii:: that seem unrelated and could be made separate.

In general, it is very very difficult to review code that is moved because one doesn't get to see what is actually different. I would very much like it if you could keep simple moves of code blocks (possibly including adjustment of class names, template lists, etc -- everything that has no functional impact) in separate commits, and to make follow-up functional changes in separate commits.

@peterrum peterrum force-pushed the dofhandler branch 2 times, most recently from ebed913 to fe9d3e7 Compare April 3, 2020 15:54
@peterrum
Copy link
Member Author

peterrum commented Apr 3, 2020

I suspect nobody is excited about reviewing this :-( Do you think you could chop off smaller pieces that could be independently merged? For example, the block_info.* changes seem relatively unrelated.

Yes, I can do that. I will create a PR for the BlockInfo and one for the instantiation of the DoFHandlers.

There are also a lot of additions of dealii:: that seem unrelated and could be made separate.

Actually, I am waiting that #9758 is merged (this contains these changes).

In general, it is very very difficult to review code that is moved because one doesn't get to see what is actually different. I would very much like it if you could keep simple moves of code blocks (possibly including adjustment of class names, template lists, etc -- everything that has no functional impact) in separate commits, and to make follow-up functional changes in separate commits.

I would suggest that I split this PR up in two parts. Part one: move the implementation of DoFHandler and hp::DoFHandler into the same files (dof_handler_base.h and dof_handler_base.templates.h) (no changes else). Part two: actually introduce DoFHandlerBase and make the DoFHandler/hp::DoFHandler implementations part of this class.

What do you think about this approach?

Generally what is your opinion of our proposal in the description?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants