Refactor part mesh tool #2159

endJunction · 2018-07-09T22:12:34Z

Major refactoring of the partmesh tool as preparation for the boundary mesh partitioning conforming to a partitioned mesh.

The refactorings try to separate the write functions from the NodeWiseMeshPartitioner class as first step, and remove code duplications in the second.
There are still some type and variable names, I'm not happy about but don't know a better name for now.

Other notable changes:

Extract the metis handling in own files.
Add -o flag for output directory.
A test for ascii and binary output of the partmesh tool is added.

Review commit-wise for understanding the changes. The changes include a new file (needed for test) which is 1176 lines, so netto we have +795 -610.

wenqing

👍

wenqing · 2018-07-10T08:59:20Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.cpp

@@ -16,6 +16,7 @@

 #include <cstdio>  // for binary output


Since fwrite is replaced with iostream::write, this include can be removed. In additional, I had ever experienced that MPI_file_read could not read the binary by iostream::write. The reason might be that MPI was complied by using old C compiler.

chleh

Looks good. Only minor comments.

chleh · 2018-07-10T12:08:18Z

Applications/Utils/ModelPreparation/PartitionMesh/PartitionMesh.cpp

-        }
-        else if (num_partitions == 1 && exe_metis_flag.getValue())
+        const std::string mpmetis_com =
+            exe_path + "/mpmetis " + " -gtype=nodal " + file_name_base +


Problem: file_name_base could contain spaces (especially if the file name is a full/relative path name).
Cf. https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152177.

The execve solution is very clumsy for this. Would adding " around the filename suffice?

... + "\"" + file_name_base + "\"" + ...;

For the time being that would already be an improvement. Adding single quotes ' might be slightly better. Maybe in the future we should write a small function for that.

Or boost for the rescue (in the future):
https://www.boost.org/doc/libs/1_67_0/doc/html/boost_process/tutorial.html#boost_process.tutorial.starting_a_process

chleh · 2018-07-10T12:09:38Z

Applications/Utils/ModelPreparation/PartitionMesh/PartitionMesh.cpp

        }
+    }
+    else if (num_partitions == 1 && exe_metis_flag.getValue())
+    {


Can't we drop that branch entirely since OGS has support for PETSc reading vtu files?

@wenqing Please comment.

As suggested by @chleh, that branch can be replaced by OGS_FATAL as

OGS_FATAL("Partitioning the mesh into one domain is unnecessary because OGS reads vtu mesh data directly when np=1 ")

chleh · 2018-07-10T12:12:47Z

Applications/Utils/ModelPreparation/PartitionMesh/Metis.cpp

+        OGS_FATAL("Error while reading file %s.", fname_parts.data());
+    }
+
+    npart_in.close();


unnecessary

chleh · 2018-07-10T12:14:12Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.h

@@ -79,6 +75,13 @@ class NodeWiseMeshPartitioner
    /// \param file_name_base The prefix of the file name.
    void writeBinary(const std::string& file_name_base);

+    void resetPartitionIdsForNodes(std::vector<std::size_t> node_partition_ids)


shouldn't the argument be an rvalue ref, too?
Probably the caller never wants to copy that arg.

chleh · 2018-07-10T12:17:10Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.cpp

+                                  coords[1], coords[2]);
+    }
+    return os.write(reinterpret_cast<const char*>(nodes_buffer.data()),
+                    sizeof(NodeStruct) * nodes_buffer.size());


Is anywhere endianness indicated?

It's in the sky ;)

chleh · 2018-07-10T13:06:06Z

Applications/Utils/Tests.cmake

+    PATH NodePartitionedMesh/partmesh_2Dmesh_3partitions
+    EXECUTABLE partmesh
+    EXECUTABLE_ARGS -a -m -n 3 -i 2Dmesh.vtu -o ${Data_BINARY_DIR}/NodePartitionedMesh/partmesh_2Dmesh_3partitions
+    REQUIREMENTS


no requirements?

Let's say no restrictions...

chleh · 2018-07-10T13:10:39Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.cpp

    }
    return true;
 }

+template <typename Function>


IMHO this code would be easier to understand if you would provide the function copy_property_vector right here abov this line and not pass it as a lambda argument.

chleh · 2018-07-10T13:15:01Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.cpp

 template <typename Function>
-void copyPropertyVectors(std::vector<std::string> const& property_names,
-                         Function copy_property_vector)
+void applyPropertyVectors(std::vector<std::string> const& property_names,


OK, It's an abstraction. But is that really needed? And maybe applyToPropertyVectors would be a better name.

chleh · 2018-07-10T13:17:53Z

MeshLib/Location.h

@@ -20,6 +20,14 @@ namespace MeshLib

 enum class MeshItemType { Node, Edge, Face, Cell, IntegrationPoint };

+/// Returns a string output for a specific error flag


MeshItemType instead of error flag? Maybe the spelling should be the same as in the enum, i.e., CamelCase.

chleh · 2018-07-10T13:19:36Z

Applications/Utils/ModelPreparation/PartitionMesh/NodeWiseMeshPartitioner.cpp

+                         [&](auto type, std::string const& name) {
+                             return writePropertyVectorBinary<decltype(type)>(
+                                 partitioned_properties, name, out_val, out);
+                         });


OK, here, some code duplication is reduced.

endJunction · 2018-07-11T20:05:27Z

Ok. Almost there. I had to remove the BL::createBinaryFile() again, because of gcc<5.

@bilke Two points for discussion: Can we see what is different on the mac results? Why is it that on the docker-conan node metis' cmake is not finding GKlib (was probably not tested before)?

This required an access function to the partion ids and the mesh.

The 2Dmesh contains higher order quad8 elements and line3 elements. Scalar and vectorial point and cell data arrays are present. This is copy of a LIE single-fracture benchmark result.

chleh · 2018-07-12T07:50:42Z

BaseLib/FileTools.cpp

-    {
-        OGS_FATAL("Could not open file '%s' for output.", file_name.c_str());
-    }
-    return os;


What's the problem? Missing copy elision?

The move operator is not implemented as I understood. https://stackoverflow.com/a/28775806

Another one not working on gcc < 5:

struct A { int a; int b = 0; }; void f() { A a = {2, 3}; } // or A g() { return {4, 5}; }

because the struct with default initializers cannot be constructed from initializer list; see https://stackoverflow.com/a/37777814
Happened here: https://jenkins.opengeosys.org/blue/organizations/jenkins/ufz%2Fogs/detail/PR-2159/13/pipeline/

I think it's time to move to minimum gcc 5.3 (5.2 had an internal compiler error in the other PR.)...

bilke · 2018-07-12T09:02:15Z

scripts/cmake/SubmoduleSetup.cmake

@@ -12,9 +12,6 @@ set(REQUIRED_SUBMODULES
    ThirdParty/tetgen
    ${OGS_ADDITIONAL_SUBMODULES_TO_CHECKOUT}
 )
-if(OGS_BUILD_METIS)
-    list(APPEND REQUIRED_SUBMODULES ThirdParty/metis)


@endJunction You need to add the submodule to the list of required submodules

bilke · 2018-07-12T09:46:52Z

@endJunction To get the diff output you may want to change https://github.com/ufz/ogs/blob/master/scripts/cmake/test/AddTestTester.cmake#L49

to

message(FATAL_ERROR "Error exit code: ${EXIT_CODE}\n${OUTPUT}")

A higher order 2D mesh with mixed quad8/line3 elements, scalar and vectorial, point and cell data is tested.

Extract both the original function from the class and a function for a partition.

It's not accessing any members of the class.

Also extract a code enumerating the partition's local nodes.

Split "config_data" into partition data and offsets.

Extract specific (to node or cell) functions, generalize some of the loops. There are still some code duplications, but more difficult to extract...

This allows to unify the processNodeProperties() and the processCellProperties().

The reinterpret cast seems to more appropriate in context of writing of binary values to a stream.

Remove code duplication and use applyPropertyVector.

CL: "It would be better to use boost::process" https://www.boost.org/doc/libs/1_67_0/doc/html/boost_process/tutorial.html#boost_process.tutorial.starting_a_process

The move ctor is not implemented in gcc < 5.

The dependencies include targets like partmesh and MapGeometryToMeshSurface which are referenced in Tests.cmake.

ogsbot · 2020-06-19T12:53:29Z

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

wenqing reviewed Jul 10, 2018

View reviewed changes

chleh approved these changes Jul 10, 2018

View reviewed changes

wenqing approved these changes Jul 10, 2018

View reviewed changes

endJunction added 8 commits July 12, 2018 00:07

Clang-format node-wise mesh partitioner and CLI.

6990775

NWMP; Fix typo in comment.

17484c6

Extract writeMETIS().

0101659

Extract readMetisData().

bb69a92

This required an access function to the partion ids and the mesh.

[T] Add partmesh complex test case input files.

7ed8ce8

The 2Dmesh contains higher order quad8 elements and line3 elements. Scalar and vectorial point and cell data arrays are present. This is copy of a LIE single-fracture benchmark result.

[T] partmesh ascii expected output.

a58f132

[T] partmesh binary expected output

2c8ad39

[App/U] Add output directory option to partmesh.

13eb89b

chleh reviewed Jul 12, 2018

View reviewed changes

bilke reviewed Jul 12, 2018

View reviewed changes

endJunction added 15 commits July 12, 2018 16:32

[T] partmesh ctests for binary and ascii.

564038b

A higher order 2D mesh with mixed quad8/line3 elements, scalar and vectorial, point and cell data is tested.

NWMP: Extract writeNodesBinary();

aba0e71

Extract both the original function from the class and a function for a partition.

NWMP; Fix splitOffHigherOrderNode() function name.

83819bf

NWMP; Simplify splitOffHigherOrderNode().

09e3604

NWMP; Extract getNumberOfIntVariablesOfElements().

c626caa

NWMP; Move out getElementIntegerVariables().

403caa7

It's not accessing any members of the class.

[BL] FileTools; Add createBinaryFile with check.

b0157a7

NWMP; Use ofstream for binary output.

eed0523

[NWMP] Extract writeElementsBinary().

42e0d09

Also extract a code enumerating the partition's local nodes.

NWMP: Extract writeConfigDataBinary().

836b5ce

NWMP; Rewrite binary config output.

bc0da8c

Split "config_data" into partition data and offsets.

NWMP; Extract write and property related functions

089b604

NWMP; Refactor copying of property vector values.

a65ebb7

Extract specific (to node or cell) functions, generalize some of the loops. There are still some code duplications, but more difficult to extract...

NWMP; Extract Partition::numberOfMeshItems.

65c8422

This allows to unify the processNodeProperties() and the processCellProperties().

[BL] Change cast type in writeValueBinary.

ff69e62

The reinterpret cast seems to more appropriate in context of writing of binary values to a stream.

endJunction added 10 commits July 12, 2018 18:11

[MeL] Add toString(MeshItemType) conversion.

77983b4

NWMP; Refactor writeNode/CellPropertiesBinary.

185aff6

Remove code duplication and use applyPropertyVector.

NWMP; Rename applyToPropertyVectors().

5fc3d3c

PM; Remove code for 1-partition.

8fcb6f4

PM; Add ticks around metis call input filename.

781a263

CL: "It would be better to use boost::process" https://www.boost.org/doc/libs/1_67_0/doc/html/boost_process/tutorial.html#boost_process.tutorial.starting_a_process

[BL] Remove fct. because of missing compiler supp.

f4917ae

The move ctor is not implemented in gcc < 5.

[jenkins] Unset CC=mpicc and CXX=mpicxx in envinf1

06fa93f

Always build metis if utils are enabled.

243ecbd

Add ctest-* dependencies if utils are build.

6db77bf

The dependencies include targets like partmesh and MapGeometryToMeshSurface which are referenced in Tests.cmake.

PM; Add mpmetis as cmake dependency of partmesh.

5df7f9b

endJunction merged commit ae82fdd into ufz:master Jul 12, 2018

endJunction deleted the RefactorPartMeshTool branch July 12, 2018 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor part mesh tool #2159

Refactor part mesh tool #2159

endJunction commented Jul 9, 2018 •

edited

Loading

wenqing left a comment

wenqing Jul 10, 2018

chleh left a comment

chleh Jul 10, 2018

endJunction Jul 10, 2018

chleh Jul 11, 2018

chleh Jul 11, 2018 •

edited

Loading

chleh Jul 10, 2018

endJunction Jul 10, 2018

wenqing Jul 11, 2018

chleh Jul 10, 2018

chleh Jul 10, 2018

chleh Jul 10, 2018

endJunction Jul 10, 2018

chleh Jul 10, 2018

endJunction Jul 10, 2018

chleh Jul 10, 2018

chleh Jul 10, 2018

chleh Jul 10, 2018

chleh Jul 10, 2018

endJunction commented Jul 11, 2018

chleh Jul 12, 2018

endJunction Jul 12, 2018

endJunction Jul 12, 2018

bilke Jul 12, 2018

bilke commented Jul 12, 2018

ogsbot commented Jun 19, 2020

		@@ -20,6 +20,14 @@ namespace MeshLib

		enum class MeshItemType { Node, Edge, Face, Cell, IntegrationPoint };

		/// Returns a string output for a specific error flag

Refactor part mesh tool #2159

Refactor part mesh tool #2159

Conversation

endJunction commented Jul 9, 2018 • edited Loading

wenqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chleh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chleh Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endJunction commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilke commented Jul 12, 2018

ogsbot commented Jun 19, 2020

endJunction commented Jul 9, 2018 •

edited

Loading

chleh Jul 11, 2018 •

edited

Loading