-
Notifications
You must be signed in to change notification settings - Fork 471
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
Exodus II Writer #4208
base: master
Are you sure you want to change the base?
Exodus II Writer #4208
Conversation
… updated cmakelists.
…enerates key information about each boundary which can then be written to the file.
…ing nc_enddef and nc_redef.
…x-coordinate from being read.
Thanks @EdwardPalmer99! |
This PR is now under review (see the table in the PR description). To help with the review process, please do not force push to the branch. |
Hi, @EdwardPalmer99, |
mesh/exodus_writer.cpp
Outdated
#ifdef MFEM_USE_NETCDF | ||
|
||
// Variable labels | ||
const char * EXODUS_TITLE_LABEL = "title"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are reluctant to place variables directly into the mfem
namespace. Even when they have such specific names as these. I think we would most often define these as static
variables within something like the ExodusIIWriter
class. If you prefer they could probably be placed within a separate utility class. Other possibilities exist. If you have a strong inclination towards a particular solution we can certainly discuss options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sure. How about plonking them into an "ExodusIILabels" namespace? I think this is a better solution than putting them into a utility class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as that namespace is inside the mfem
namespace I think that would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps these mesh files (which add up to about 0.5 MB) should be moved to the mfem/data repository. The unit tests can then be tagged with the tag [MFEMData]
, so that they will only be run if the unit test executable is passed the --data
command line argument.
void CloseExodusII(); | ||
|
||
/// @brief Generates blocks based on the elements in the mesh. We iterate | ||
/// over the mehs elements and use the attributes as the element blocks. We |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: mehs
// for the block. | ||
if (element_type != _element_type_for_block_id.at(block_id)) | ||
{ | ||
MFEM_ABORT("Multiple element types are defined for block: " << block_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we change this so that the attempt to write an output file stops but program execution can continue. We could print a warning message stating either that mixed meshes are not yet supported or that Exodus II meshes do not support mixed element types. Whichever is more accurate.
While we're on this topic: if mixed meshes are supported but mixed blocks are not then I would like to see an outline of how we might improve such support in the future. Might it be as simple as pre-processing the mesh and dividing regions with a common attribute number into multiple blocks based on element type?
// 6. Define the element type. | ||
std::string element_type; | ||
|
||
bool higher_order = (_mesh.GetNodes() != nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be a little more discerning here. At the very least we should confirm that the Nodes
defined in the mesh do indeed correspond to a 2nd order H1 space (because that would seem to be the assumption made later on). We can then exit gracefully with a warning message if printing this particular type of mesh is not supported.
In the future we can perhaps address issues such as Nodes
based on L2 spaces. We could even project higher order Nodes
to 2nd order nodes as long as we print a warning message. These improvements do not need to be handled at this time but we should be clear about the limitations of the current implementation.
int bdr_element_face_index = _mesh.GetBdrElementFaceIndex(ibdr_element); | ||
|
||
// Locate match. | ||
auto & element_face_info = mfem_face_index_info_for_global_face_index.at( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain but I believe this call to at
may be failing on interior surfaces such as those found in miniapps/multidomain/multidomain-hex.mesh
. Some would argue that "boudary" faces should never exist within the interior of a mesh but MFEM meshes do support this. Unfortunately this means that this writer must determine how best to handle such things. Does the Exodus II format support interior "boundary" faces? Does it store such things in a different manner? At the very least the current implementation of this writer should recognize these "interior" boundary faces and skip them if necessary (and issue a warning message to that effect).
Brief
First-Order Element Types
Second-Order Element Types
Caveats
Future Work
PR Checklist
make style
.CHANGELOG
:CHANGELOG
to group with other related features?INSTALL
:make
orcmake
have a new target?.github
.appveyor.yml
.gitignore
:make distclean; git status
shows any files that were generated from the source by the project (not an IDE) but we don't want to track in the repository.examples/makefile
:SEQ_EXAMPLES
andPAR_EXAMPLES
variables.clean
target..gitignore
file.examples/CMakeLists.txt
:ALL_EXE_SRCS
variable.THIS_TEST_OPTIONS
is set correctly for the new example.doc/CodeDocumentation.dox
.examples/pumi
), list it indoc/CodeDocumentation.conf.in
src/examples.md
.src/examples.md
andsrc/img
.examples.md
, list the example under the appropriate categories, add new categories if necessary.features.md
.makefile
andmakefile
in corresponding miniapp directory..gitignore
file.CMakeLists.txt
file in theminiapps
directory, if the new miniapp is in a new directory.CMakeLists.txt
file in the new miniapp directory.doc/CodeDocumentation.dox
miniapps/nurbs
), add it toMINIAPP_SUBDIRS
in themakefile
.miniapps/nurbs
), list it indoc/CodeDocumentation.conf.in
src/meshing.md
andsrc/electromagnetics.md
files.src/examples.md
andsrc/img
.features.md
.mfem/web
repo.README
(rare).doc/CodeDocumentation.dox
(rare).make unittest
to make sure all unit tests pass.tests/scripts
.