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

Refactor part mesh tool #2159

Merged
merged 33 commits into from Jul 12, 2018

Conversation

Projects
None yet
4 participants
@endJunction
Copy link
Member

endJunction commented Jul 9, 2018

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.

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch 2 times, most recently from 126fcf1 to ccbff28 Jul 9, 2018

@wenqing
Copy link
Member

wenqing left a comment

👍

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

#include <cstdio> // for binary output

This comment has been minimized.

@wenqing

wenqing Jul 10, 2018

Member

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

chleh approved these changes Jul 10, 2018

Copy link
Collaborator

chleh left a comment

Looks good. Only minor comments.

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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.

This comment has been minimized.

@endJunction

endJunction Jul 10, 2018

Author Member

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

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

This comment has been minimized.

@chleh

chleh Jul 11, 2018

Collaborator

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.

This comment has been minimized.

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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

This comment has been minimized.

@endJunction

endJunction Jul 10, 2018

Author Member

@wenqing Please comment.

This comment has been minimized.

@wenqing

wenqing Jul 11, 2018

Member

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 ")
OGS_FATAL("Error while reading file %s.", fname_parts.data());
}

npart_in.close();

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

unnecessary

@@ -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)

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

Is anywhere endianness indicated?

This comment has been minimized.

@endJunction

endJunction Jul 10, 2018

Author Member

It's in the sky ;)

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

no requirements?

This comment has been minimized.

@endJunction

endJunction Jul 10, 2018

Author Member

Let's say no restrictions...

}
return true;
}

template <typename Function>

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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.

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,

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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

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

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

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

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

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

This comment has been minimized.

@chleh

chleh Jul 10, 2018

Collaborator

OK, here, some code duplication is reduced.

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch 4 times, most recently from ce31291 to d9a748f Jul 10, 2018

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jul 11, 2018

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)?

endJunction added some commits Jul 6, 2018

Extract readMetisData().
This required an access function to the partion ids
and the mesh.
[T] Add partmesh complex test case input files.
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.

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch from d9a748f to 57d1c7d Jul 11, 2018

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

This comment has been minimized.

@chleh

chleh Jul 12, 2018

Collaborator

What's the problem? Missing copy elision?

This comment has been minimized.

@endJunction

endJunction Jul 12, 2018

Author Member

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

This comment has been minimized.

@endJunction

endJunction Jul 12, 2018

Author Member

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.)...

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

This comment has been minimized.

@bilke

bilke Jul 12, 2018

Member

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

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Jul 12, 2018

@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}")

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch from 9048614 to 398ea39 Jul 12, 2018

endJunction added some commits Jul 7, 2018

[NWMP] Extract writeElementsBinary().
Also extract a code enumerating the partition's local
nodes.
[T] partmesh ctests for binary and ascii.
A higher order 2D mesh with mixed quad8/line3 elements,
scalar and vectorial, point and cell data is tested.

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch from 398ea39 to f555245 Jul 12, 2018

endJunction added some commits Jul 7, 2018

NWMP; Rewrite binary config output.
Split "config_data" into partition data and offsets.
NWMP; Refactor copying of property vector values.
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.
This allows to unify the processNodeProperties() and
the processCellProperties().
[BL] Change cast type in writeValueBinary.
The reinterpret cast seems to more appropriate in context
of writing of binary values to a stream.
NWMP; Refactor writeNode/CellPropertiesBinary.
Remove code duplication and use applyPropertyVector.
Add ctest-* dependencies if utils are build.
The dependencies include targets like partmesh and
MapGeometryToMeshSurface which are referenced in
Tests.cmake.
[BL] Remove fct. because of missing compiler supp.
The move ctor is not implemented in gcc < 5.

@endJunction endJunction force-pushed the endJunction:RefactorPartMeshTool branch from f555245 to 5df7f9b Jul 12, 2018

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details

@endJunction endJunction deleted the endJunction:RefactorPartMeshTool branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.