trixi-framework / HOHQMesh

High Order Hex-Quad Mesh (HOHQMesh) package to automatically generate all-quadrilateral meshes with high order boundary information.
https://trixi-framework.github.io/HOHQMesh
Other
45 stars 8 forks source link

Add feature to create bottom topography from data #27

Closed andrewwinters5000 closed 2 years ago

andrewwinters5000 commented 2 years ago

This is a small step to add the capability of creating a bottom topography from data read in from a file. Then a 3D mesh is created using simple extrusion. To create the high-order boundary information a bicubic interpolation is used given the available data read in from the provided file.

Currently, this file data is assumed to be gridded data that has (presumably) been pre-processed by a user.

codecov[bot] commented 2 years ago

Codecov Report

Merging #27 (9e79ef2) into main (98e2631) will increase coverage by 9.34%. The diff coverage is 92.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   57.49%   66.83%   +9.34%     
==========================================
  Files          65       66       +1     
  Lines        9580     9927     +347     
==========================================
+ Hits         5508     6635    +1127     
+ Misses       4072     3292     -780     
Flag Coverage Δ
unittests 66.83% <92.01%> (+9.34%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Source/IO/MeshOutputMethods.f90 69.78% <ø> (ø)
Source/Mesh/LaplaceMeshSmoother.f90 0.00% <ø> (ø)
Source/Mesh/SpringMeshSmoother.f90 92.56% <ø> (+1.35%) :arrow_up:
Source/Project/MeshProject.f90 72.36% <ø> (+4.27%) :arrow_up:
...rce/Surfaces/ParametricEquationTopographyClass.f90 29.78% <ø> (+29.78%) :arrow_up:
Source/HOHQMesh.f90 66.11% <6.25%> (+10.04%) :arrow_up:
Source/Surfaces/DataFileTopographyClass.f90 88.46% <88.46%> (ø)
Source/Testing/MeshingTests.f90 90.81% <89.47%> (-2.78%) :arrow_down:
Source/Testing/TestDataClass.f90 68.42% <94.44%> (+20.80%) :arrow_up:
Source/3DSource/3DMeshController.f90 94.00% <100.00%> (+90.00%) :arrow_up:
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 98e2631...9e79ef2. Read the comment docs.

fluidnumerics-joe commented 2 years ago

@andrewwinters5000 - I just pushed up a patch that resolves the compiling issue with the exception handler call. I think we now want to settle on who to put as the poster

fluidnumerics-joe commented 2 years ago

@DavidAKopriva @andrewwinters5000 it looks like there are some coverage issues that can be resolved. I'll take a look at where we can improve coverage; I suspect we don't have tests running for the new example associated with this feature. It'd be great to get this merged in

andrewwinters5000 commented 2 years ago

I'll take a look at where we can improve coverage; I suspect we don't have tests running for the new example associated with this feature. It'd be great to get this merged in

I agree it would be good to get this feature merged into main. Do we need to merge your spline_curve feature first @fluidnumerics-joe ? Or are the two features orthogonal in some sense?

andrewwinters5000 commented 2 years ago

The coverage improved because the new 3D test also exercises the Abaqus output routines in 3DSource/Mesh3DOutputMethods.f90. It is strange that the destruct and release routines for the new SMTopographyFromFile are not covered. I believe that these should get invoked when lines 173-176 in destructModel are executed because that is when the topography pointer gets released. Do I understand this release call correctly @DavidAKopriva ?

andrewwinters5000 commented 2 years ago

mountain

DavidAKopriva commented 2 years ago

Andrew,

  1. The "how to add a test case" looks good. I'm almost wondering if the list of tests should be in a file rather than code, since there is actually no reason to have to edit code to add a test case. OTOH, there's one less file path to have to maintain.
  2. In the docs for getting the topography from a file, there is no example showing how to invoke it. I presume it's "file =" in the TOPOGRAPHY block. Will start looking through the code next...
andrewwinters5000 commented 2 years ago
  • The "how to add a test case" looks good. I'm almost wondering if the list of tests should be in a file rather than code, since there is actually no reason to have to edit code to add a test case. OTOH, there's one less file path to have to maintain.

It might be easiest to move it out of the code and into a separate file. Having it directly in the MeshTesting.f90 file tends to regularly cause merge conflicts.

  • In the docs for getting the topography from a file, there is no example showing how to invoke it. I presume it's "file =" in the TOPOGRAPHY block.

Whoops, totally forgot an example invoking it. I will add that.

fluidnumerics-joe commented 2 years ago

It might be easiest to move it out of the code and into a separate file. Having it directly in the MeshTesting.f90 file tends to regularly cause merge conflicts.

I agree with moving the list of tests to a file that just lists the set of control files under Benchmarks. Alternatively, if we don't want to maintain a list, we could modify the ci.yaml to create such a list on the fly by listing the control files under Benchmarks/ControlFiles.

If we need to require that a test file is available for comparison, we could modify the MeshingTests to throw an error if one is not found.

DavidAKopriva commented 2 years ago

"It might be easiest to move it out of the code and into a separate file. Having it directly in the MeshTesting.f90 file tends to regularly cause merge conflicts." --- That's my thinking, too. Let's finish up with this PR and think about that in the meantime. The issue will come up again when we merge the TopographyResolution branch in, will I will propose to look at after this one.

andrewwinters5000 commented 2 years ago

That's my thinking, too. Let's finish up with this PR and think about that in the meantime. The issue will come up again when we merge the TopographyResolution branch in, will I will propose to look at after this one.

That's a good plan especially because this PR already has a lot in it.

DavidAKopriva commented 2 years ago

I'll go though the new stuff this morning, esp. the fact that the final routines are not being called.

DavidAKopriva commented 2 years ago

Thanks for noticing the fact the destructors weren't called. The bug was way up in the chain where the FINAL routine for the MeshProject class wasn't defined.