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

Bug fixes PolyhedralGrid #513

Merged
merged 5 commits into from
Feb 3, 2021
Merged

Conversation

rbe051
Copy link
Contributor

@rbe051 rbe051 commented Feb 2, 2021

It was suggested that #512 should be split in two parts; one for the polyhedral grid bugfixes and one for the vertical equilibrium grid. This pull request contains the bugfixes to the polyhedral grid.

The main two fixes in this pull request is to
1, make the PolyhedralGrid work for embedded lower dimensional grids
2, assign a geometry type to all codimensions (~ line 1600)

rube051 added 3 commits February 1, 2021 12:07
In 1d faces and nodes have the same codimension. We assume that
the unstructured grid has a one to one mapping between the faces
and the nodes
This commit contains two bugfixes:
- The geometry type was only set to codim 0 and 1.
- To make the PolyhedralGrid work for lower dimensional grids embedded
in a higher dimensional domain (e.g., 2d grid in 3d domain), the
UnstructuredGrid dimension must equal the domain dimension. Then
all coordinates are expressed in global coordinates.
return grid_.face_nodepos[ index+1 ] - grid_.face_nodepos[ index ];
if (codim==dim)
return 1;
return 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is necessary to make PolyhedralGrid work for 1D grids (then dim = 1, and two cases in the switch are equal). In this case faces and nodes have the same co-dimension. With the current change we always return the face properties for codim=1. If the grid dimension is 1, should we rather return the node properties? I'm not sure if it matters as there is (always?) a one-to-one mapping between faces and nodes in 1D.

@blattms
Copy link
Member

blattms commented Feb 2, 2021

One question:
If we are using a 1D grid, of what size will the C-arrayface_nodepos of UnstructuredGrid be?

@blattms
Copy link
Member

blattms commented Feb 2, 2021

Should have figured that out before posting the question. It is the number of faces as each face is a node.

@blattms
Copy link
Member

blattms commented Feb 2, 2021

jenkins build this please

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and the additional check.

You might want to check with valgrind and investigate, though. For this branch it complains
about missmatched frees when running test_polyhedralgrid.cpp. Here is the additional output when comparing to master:

0
0
Lifetime / consistency check for entities, codim 2
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 1 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 1 elements
Lifetime / consistency check for entities, codim 2
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 1 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 1 elements
==120816== Mismatched free() / delete / delete []
==120816==    at 0x4836F9E: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
==120816==    by 0x7DFCD2: main (test_polyhedralgrid.cpp:379)
==120816==  Address 0x5fb70050 is 0 bytes inside a block of size 144 alloc'd
==120816==    at 0x4835753: malloc (vg_replace_malloc.c:299)
==120816==    by 0x96E8AE: create_grid_empty (UnstructuredGrid.c:64)
==120816==    by 0x96EA26: allocate_grid (UnstructuredGrid.c:97)
==120816==    by 0x96EE5B: allocate_grid_from_file (UnstructuredGrid.c:216)
==120816==    by 0x96F84E: read_grid (UnstructuredGrid.c:538)
==120816==    by 0x7DFBF2: main (test_polyhedralgrid.cpp:368)
==120816== 
0
0
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 2 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 2 elements
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 2 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 2 elements
==120816== Mismatched free() / delete / delete []
==120816==    at 0x4836F9E: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
==120816==    by 0x7E013E: main (test_polyhedralgrid.cpp:415)
==120816==  Address 0x5fb78670 is 0 bytes inside a block of size 144 alloc'd
==120816==    at 0x4835753: malloc (vg_replace_malloc.c:299)
==120816==    by 0x96E8AE: create_grid_empty (UnstructuredGrid.c:64)
==120816==    by 0x96EA26: allocate_grid (UnstructuredGrid.c:97)
==120816==    by 0x96EE5B: allocate_grid_from_file (UnstructuredGrid.c:216)
==120816==    by 0x96F84E: read_grid (UnstructuredGrid.c:538)
==120816==    by 0x7E0067: main (test_polyhedralgrid.cpp:404)
==120816== 
0
0
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 2 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 2 elements
Lifetime / consistency check for entities, codim 1
WARNING! Requested check of first 32 entities, but grid view only contains 3 entities
Lifetime / consistency check for entities, codim 0
WARNING! Requested check of first 32 entities, but grid view only contains 2 entities
Intersection Lifetime / consistency check
WARNING! Requested check of intersections for first 32 elements, but grid view only contains 2 elements
==120816== Mismatched free() / delete / delete []
==120816==    at 0x4836F9E: operator delete(void*, unsigned long) (vg_replace_malloc.c:585)
==120816==    by 0x7E0598: main (test_polyhedralgrid.cpp:451)
==120816==  Address 0x5fb80190 is 0 bytes inside a block of size 144 alloc'd
==120816==    at 0x4835753: malloc (vg_replace_malloc.c:299)
==120816==    by 0x96E8AE: create_grid_empty (UnstructuredGrid.c:64)
==120816==    by 0x96EA26: allocate_grid (UnstructuredGrid.c:97)
==120816==    by 0x96EE5B: allocate_grid_from_file (UnstructuredGrid.c:216)
==120816==    by 0x96F84E: read_grid (UnstructuredGrid.c:538)
==120816==    by 0x7E04C1: main (test_polyhedralgrid.cpp:440)
==120816== 
==120816== 
==120816== HEAP SUMMARY:
==120816==     in use at exit: 23,172 bytes in 126 blocks
==120816==   total heap usage: 466,764 allocs, 466,638 frees, 20,392,434 bytes allocated
==120816== 
==120816== LEAK SUMMARY:
==120816==    definitely lost: 1,217 bytes in 43 blocks
==120816==    indirectly lost: 0 bytes in 0 blocks
==120816==      possibly lost: 10,912 bytes in 31 blocks
==120816==    still reachable: 11,043 bytes in 52 blocks
==120816==         suppressed: 0 bytes in 0 blocks
==120816== Rerun with --leak-check=full to see details of leaked memory
==120816== 
==120816== For counts of detected and suppressed errors, rerun with: -v
==120816== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

Comment on lines 1606 to 1615
if( hasSimplex ){
tmp = Dune::GeometryTypes::simplex(dim - codim);
geomTypes_[ codim ].push_back( tmp );
}
else if ( hasCube ){
tmp = Dune::GeometryTypes::cube(dim - codim);
geomTypes_[ codim ].push_back( tmp );}
else if (hasPolyhedron){
tmp = Dune::GeometryTypes::none(dim - codim);
geomTypes_[ codim ].push_back( tmp );}
Copy link
Member

@blattms blattms Feb 2, 2021

Choose a reason for hiding this comment

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

Nitpick: Formatting is messed up (not enough indentation and } on the same line) which makes this hard to read.

rube051 added 2 commits February 2, 2021 17:19
@rbe051
Copy link
Contributor Author

rbe051 commented Feb 2, 2021

Thanks for the review.

I have updated the formatting and tried to fixe the memory leaks. Valgrind is still complaining about possible memory leaks, however, I get the same results when testing on master.

==183467== 
==183467== HEAP SUMMARY:
==183467==     in use at exit: 7,584 bytes in 15 blocks
==183467==   total heap usage: 433,796 allocs, 433,781 frees, 15,130,718 bytes allocated
==183467== 
==183467== LEAK SUMMARY:
==183467==    definitely lost: 0 bytes in 0 blocks
==183467==    indirectly lost: 0 bytes in 0 blocks
==183467==      possibly lost: 3,344 bytes in 11 blocks
==183467==    still reachable: 4,240 bytes in 4 blocks
==183467==         suppressed: 0 bytes in 0 blocks
==183467== Rerun with --leak-check=full to see details of leaked memory
==183467== 
==183467== For lists of detected and suppressed errors, rerun with: -s
==183467== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

thanks for the changes. The double frees are gone and I think this got to go in (if jenkins agrees).

@blattms
Copy link
Member

blattms commented Feb 2, 2021

jenkins build this please

@blattms blattms merged commit 750a7ed into OPM:master Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants