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

Geometry based on mapping by Lagrange basis functions #851

Merged

Conversation

CsatiZoltan
Copy link
Collaborator

@CsatiZoltan CsatiZoltan commented Nov 15, 2023

This PR will implement an alternative volume mapping, see also #794.
Eventually, this contribution will provide the numerics for the mapping, i.e. points 1 and 3 in Sandro Elsweijer's comment. This PR is not concerned with the creation of a mesh reader. I plan to include

  • several element types (T6, Q4, etc.) in 2D and 3D
  • tests for all these element types
  • examples/tutorials
  • documentation
  • potentially a method to generate the basis functions for arbitrary-order Lagrange finite elements

This is my first PR, so I do not yet know the structure of t8code. So please forgive me if the initial structuring of the code is inappropriate. Moreover, while I read the existing code and the contribution guidelines, I am beginner in C++.


All these boxes must be checked by the reviewers before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

General

  • The reviewer executed the new code features at least once and checked the results manually

  • The code follows the t8code coding guidelines

  • New source/header files are properly added to the Makefiles

  • The code is well documented

  • All function declarations, structs/classes and their members have a proper doxygen documentation

  • All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue)

Tests

  • The code is covered in an existing or new test case using Google Test

Github action

  • The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)

  • All tests pass (in various configurations, this should be executed automatically in a github action)

    If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

    • Should this use case be added to the github action?
    • If not, does the specific use case compile and all tests pass (check manually)

Scripts and Wiki

  • If a new directory with source-files is added, it must be covered by the script/find_all_source_files.scp to check the indentation of these files.
  • If this PR introduces a new feature, it must be covered in an example/tutorial and a Wiki article.

Licence

  • The author added a BSD statement to doc/ (or already has one)

Evaluation for general elements and concrete mappings for T3, T6 and Q4 elements.
@sandro-elsweijer sandro-elsweijer self-assigned this Nov 15, 2023
@sandro-elsweijer sandro-elsweijer added New feature Adds a new feature to the code draft Enhance the visibility that this is a draft. labels Nov 15, 2023
Copy link
Collaborator

@sandro-elsweijer sandro-elsweijer left a comment

Choose a reason for hiding this comment

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

Very nice implementation here. Maybe the files should get a more telling file name, like t8_geometry_lagrangian.cxx/hxx. Furthermore, we require a C wrapping for all our CPP concepts. For that, you can adapt the t8_geometry_linear.h and the according lines in the t8_geometry_linear.cxx

Zoltan added 8 commits November 20, 2023 17:17
Although each tree could get its own geometry, this is not the t8code way of doing it. Normally, every tree should get the same geometry and then the geometry itself dispatches on the tree shape. This strategy has already been used in the linear OCC geometries.
Copy link
Collaborator

@sandro-elsweijer sandro-elsweijer left a comment

Choose a reason for hiding this comment

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

This looks very good! A batch computation would be nice, and I suggest some rigorous consting (I hope all those consts do not break the code), but the rest looks very mergeable

Copy link
Collaborator

@sandro-elsweijer sandro-elsweijer left a comment

Choose a reason for hiding this comment

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

Hey Zoltan, apart from some minor things, this looks already merge-able!
However, a test would be nice.

@sandro-elsweijer
Copy link
Collaborator

I just realized, that we have to add you implementations into the Makefile as well! You can orient yourself by the other geometries.

@CsatiZoltan CsatiZoltan marked this pull request as ready for review March 8, 2024 14:50
@CsatiZoltan CsatiZoltan force-pushed the feature-geometry_with_FE_basis branch from e900b82 to 067ba4b Compare March 8, 2024 15:23
Copy link
Collaborator

@sandro-elsweijer sandro-elsweijer left a comment

Choose a reason for hiding this comment

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

Nice work! Proposed only minor improvements.

test/t8_geometry/t8_gtest_geometry_lagrange.cxx Outdated Show resolved Hide resolved
test/t8_geometry/t8_gtest_geometry_lagrange.cxx Outdated Show resolved Hide resolved
test/t8_geometry/t8_gtest_geometry_lagrange.cxx Outdated Show resolved Hide resolved
test/t8_geometry/t8_gtest_geometry_lagrange.cxx Outdated Show resolved Hide resolved
Copy link
Collaborator

@sandro-elsweijer sandro-elsweijer left a comment

Choose a reason for hiding this comment

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

Just these two last things

src/t8_cmesh/t8_cmesh_types.h Outdated Show resolved Hide resolved
@sandro-elsweijer sandro-elsweijer removed the draft Enhance the visibility that this is a draft. label Mar 15, 2024
auto-merge was automatically disabled March 15, 2024 09:22

Head branch was pushed to by a user without write access

@sandro-elsweijer sandro-elsweijer merged commit 7ee9008 into DLR-AMR:main Mar 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Adds a new feature to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants