Created a component test for vtk writer and reader #1361
Conversation
|
@Davknapp We had a call with @lukasdreyer about the negative volume check, since it resulted in massive problems. It was poorly implemented for 2D elements and we decided to remove the poor implementation and make the complete check optional. The reasoning is that we should not fiddle with the input data of the user unasked (like when we reorder nodes in an element with negative volume) and instead we provide the validate geometry function which can still be used for checking of negative volumes. |
sandro-elsweijer
force-pushed
the
feature-vtk_meshreader
branch
from
9f91688 to
8777830
Compare
| */ | ||
| /* TODO: write a test for this function */ | ||
| int | ||
| t8_cmesh_tree_vertices_negative_volume (const t8_eclass_t eclass, const double *vertices, const int num_vertices); |
There was a problem hiding this comment.
I don't think that we should remove this function entirely. Even though it is not used internally at the moment this still can be a handy function for others.
|
|
||
| /** Check whether a cmesh is empty on all processes. | ||
| * \param [in] cmesh A committed cmesh. | ||
| * \return True (non-zero) if and only if the cmesh has trees at all. | ||
| */ | ||
|
|
||
| int |
There was a problem hiding this comment.
please update the documentation.
| @@ -622,63 +622,6 @@ t8_cmesh_msh_file_2_read_eles (t8_cmesh_t cmesh, FILE *fp, sc_hash_t *vertices, | |||
| tree_vertices[3 * t8_vertex_num + 1] = (*found_node)->coordinates[1]; | |||
| tree_vertices[3 * t8_vertex_num + 2] = (*found_node)->coordinates[2]; | |||
| } | |||
There was a problem hiding this comment.
please open up an Issue to remind us that we should think about the conditions a cmesh should fullfill (e.g. negative volume, a certain ordering of vertices.... ).
src/t8_cmesh/t8_cmesh_trees.cxx
Outdated
| { | ||
| /* Get the treea and their face neighbors */ | ||
|
|
||
| t8_locidx_t *face_neighborsa, *face_neighborsb; |
There was a problem hiding this comment.
please declare the variable at the point they are used, not at the top of the functions. I know, in these files a lot of functions declare the variables at the top, but changed our coding guidelines. These functions are not up to date and might be changed in the future to the current guidelines.
| { | ||
| int is_equal; | ||
| t8_locidx_t num_trees, num_ghost, itree, ighost; | ||
| t8_locidx_t num_trees, num_ghost, ighost; |
There was a problem hiding this comment.
please declare the variables at the point where they are used.
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (unpartitioned_cmesh_write); | ||
| t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (unpartitioned_cmesh_read); | ||
| ASSERT_EQ (num_trees_cmesh_1, num_trees_cmesh_2); | ||
| EXPECT_TRUE (t8_cmesh_is_equal_ext (unpartitioned_cmesh_write, unpartitioned_cmesh_read, 0)); |
There was a problem hiding this comment.
Can you transform this into a Makro EXPECT_CMESH_EQ ?
| t8_cmesh_commit (unpartitioned_cmesh_read, sc_MPI_COMM_WORLD); | ||
|
|
||
| /* check equality of repartioned cmeshes */ | ||
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (unpartitioned_cmesh_write); |
There was a problem hiding this comment.
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (unpartitioned_cmesh_write); | |
| const t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (unpartitioned_cmesh_write); |
|
|
||
| /* check equality of repartioned cmeshes */ | ||
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (unpartitioned_cmesh_write); | ||
| t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (unpartitioned_cmesh_read); |
There was a problem hiding this comment.
| t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (unpartitioned_cmesh_read); | |
| const t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (unpartitioned_cmesh_read); |
| t8_cmesh_unref (&unpartitioned_cmesh_write); | ||
| } | ||
| else { /* continue normally without repartitioning */ | ||
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (cmesh); |
There was a problem hiding this comment.
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (cmesh); | |
| const t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (cmesh); |
| } | ||
| else { /* continue normally without repartitioning */ | ||
| t8_gloidx_t num_trees_cmesh_1 = t8_cmesh_get_num_trees (cmesh); | ||
| t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (cmesh2); |
There was a problem hiding this comment.
| t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (cmesh2); | |
| const t8_gloidx_t num_trees_cmesh_2 = t8_cmesh_get_num_trees (cmesh2); |
Describe your changes here:
Addition of a test that creates a vtk file from a cmesh, reads that file and compares the read file with the original cmesh
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
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):
Scripts and Wiki
script/find_all_source_files.scpto check the indentation of these files.License
doc/(or already has one)Tag Label