-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
Allow specifying attribute name when reading meshtag #3257
base: main
Are you sure you want to change the base?
Changes from all commits
9c22fac
4efecce
361a202
983af85
9c31b90
6950dd0
b7fa6c4
cf379a4
ca55e25
fc18895
6075ef4
fa3a840
2501a9e
dbf155a
26aed10
7719809
b4a5187
47e9fdc
60829f0
af42617
9df7574
90d5b48
b4ed95a
3ed4bf7
edd09a1
5b13c3b
c4f87c6
80fe197
ae2d766
4457147
3bbeb7e
7ee33ec
48e0835
9c4c8cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -334,8 +334,9 @@ template void XDMFFile::write_meshtags(const mesh::MeshTags<std::int32_t>&, | |
/// @endcond | ||
//----------------------------------------------------------------------------- | ||
mesh::MeshTags<std::int32_t> | ||
XDMFFile::read_meshtags(const mesh::Mesh<double>& mesh, std::string name, | ||
std::string xpath) | ||
XDMFFile::read_meshtags_by_name(const mesh::Mesh<double>& mesh, | ||
std::string name, std::string attribute_name, | ||
std::string xpath) | ||
{ | ||
spdlog::info("XDMF read meshtags ({})", name); | ||
pugi::xml_node node = _xml_doc->select_node(xpath.c_str()).node(); | ||
|
@@ -348,8 +349,39 @@ XDMFFile::read_meshtags(const mesh::Mesh<double>& mesh, std::string name, | |
|
||
const auto [entities, eshape] = read_topology_data(name, xpath); | ||
|
||
pugi::xml_node values_data_node | ||
= grid_node.child("Attribute").child("DataItem"); | ||
pugi::xml_node attribute_node = grid_node.child("Attribute"); | ||
pugi::xml_node values_data_node = attribute_node.child("DataItem"); | ||
if (!attribute_name.empty()) | ||
{ | ||
// Search for an attribute by the name of "Name" and whose value is the | ||
// provided `attribute_name`. | ||
bool found = false; | ||
|
||
// Keep searching until it hasn't been found and there are more attribute | ||
// nodes to search. | ||
while (!found and attribute_node) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this just iterating through the attributes? If yes, can it be made simpler using pugixml functionality, see https://pugixml.org/docs/manual.html#access.basic. The code is hard to follow - at a minimum it needs a comment on what it is doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember looking at this. The problem was, if I recall correctly, that I am searching for an attribute by the name of I will add documentation but the code doesn't look to exotic to me [because I wrote it, of course XD]. |
||
{ | ||
pugi::xml_attribute hint; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's something that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hint is not set or re-used - does the code compile without it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it can because |
||
// Get the next attribute node | ||
pugi::xml_attribute name = attribute_node.attribute("Name", hint); | ||
// If it has the right name | ||
if (name.value() == attribute_name) | ||
{ | ||
// Note it down and end the search | ||
values_data_node = attribute_node.child("DataItem"); | ||
found = true; | ||
} | ||
attribute_node = attribute_node.next_sibling("Attribute"); | ||
} | ||
|
||
// If the search ended after testing all attributes but without finding | ||
// a match, throw an error. | ||
if (!found) | ||
{ | ||
throw std::runtime_error("Attribute with name '" + attribute_name | ||
+ "' not found."); | ||
} | ||
} | ||
const std::vector values = xdmf_utils::get_dataset<std::int32_t>( | ||
_comm.comm(), values_data_node, _h5_id); | ||
|
||
|
@@ -388,6 +420,13 @@ XDMFFile::read_meshtags(const mesh::Mesh<double>& mesh, std::string name, | |
return meshtags; | ||
} | ||
//----------------------------------------------------------------------------- | ||
mesh::MeshTags<std::int32_t> | ||
XDMFFile::read_meshtags(const mesh::Mesh<double>& mesh, std::string name, | ||
std::string xpath) | ||
{ | ||
return read_meshtags_by_name(mesh, name, std::string(), xpath); | ||
} | ||
//----------------------------------------------------------------------------- | ||
std::pair<mesh::CellType, int> XDMFFile::read_cell_type(std::string grid_name, | ||
std::string xpath) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,9 +164,21 @@ class XDMFFile | |
const mesh::Geometry<T>& x, std::string geometry_xpath, | ||
std::string xpath = "/Xdmf/Domain"); | ||
|
||
/// Read MeshTags by name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit confusing - the docstring says "Read MeshTags by name" but the function name is "read_meshtags_by_label". Is it by name or by label? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue comes from the following fact: mesh tags are identified by "a word", so it is natural that this word be called the "name" of the mesh tag. The problem is that there is already another There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, re-reading this conversation I noticed that in the first review, four months ago, you asked me to change the method name to avoid the word "name" in favour of "label". I had forgotten about that and, based on your comment, I reverted the change. Which one should I keep? To a user, a mesh tag's "label" means presumably very little... |
||
/// @param[in] mesh The Mesh that the data is defined on | ||
/// @param[in] name Name of the grid node in the xml file. E.g. "Material" in | ||
/// Grid Name="Material" GridType="Uniform" | ||
/// @param[in] attribute_name The name of the attribute to read | ||
/// @param[in] xpath XPath where MeshTags Grid is stored in file | ||
mesh::MeshTags<std::int32_t> | ||
read_meshtags_by_name(const mesh::Mesh<double>& mesh, std::string name, | ||
std::string attribute_name, | ||
std::string xpath = "/Xdmf/Domain"); | ||
|
||
/// Read MeshTags | ||
/// @param[in] mesh The Mesh that the data is defined on | ||
/// @param[in] name | ||
/// @param[in] name Name of the grid node in the xml file. E.g. "Material" in | ||
/// Grid Name="Material" GridType="Uniform" | ||
/// @param[in] xpath XPath where MeshTags Grid is stored in file | ||
mesh::MeshTags<std::int32_t> | ||
read_meshtags(const mesh::Mesh<double>& mesh, std::string name, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
// Copyright (C) 2024 Massimiliano Leoni | ||
// | ||
// This file is part of DOLFINx (https://www.fenicsproject.org) | ||
// | ||
// SPDX-License-Identifier: LGPL-3.0-or-later | ||
// | ||
// Unit tests for reading meshtags by name | ||
|
||
#include <catch2/catch_template_test_macros.hpp> | ||
#include <catch2/catch_test_macros.hpp> | ||
#include <dolfinx/common/IndexMap.h> | ||
#include <dolfinx/common/MPI.h> | ||
#include <dolfinx/io/XDMFFile.h> | ||
#include <dolfinx/la/Vector.h> | ||
#include <dolfinx/mesh/MeshTags.h> | ||
#include <dolfinx/mesh/generation.h> | ||
#include <dolfinx/mesh/utils.h> | ||
|
||
#include <numeric> | ||
#include <string> | ||
|
||
using namespace dolfinx; | ||
|
||
namespace | ||
{ | ||
|
||
void test_read_named_meshtags() | ||
{ | ||
const std::string mesh_file = "Domain.xdmf"; | ||
|
||
constexpr std::int32_t domain_value = 1; | ||
constexpr std::int32_t material_value = 2; | ||
|
||
// Create mesh | ||
auto part = mesh::create_cell_partitioner(mesh::GhostMode::none); | ||
auto mesh = std::make_shared<mesh::Mesh<double>>( | ||
mesh::create_rectangle(MPI_COMM_WORLD, {{{0.0, 0.0}, {1.0, 1.0}}}, {3, 3}, | ||
mesh::CellType::triangle, part)); | ||
|
||
const std::int32_t n_cells = mesh->topology()->index_map(2)->size_local(); | ||
std::vector<std::int32_t> indices(n_cells); | ||
std::iota(std::begin(indices), std::end(indices), 0); | ||
|
||
std::vector<std::int32_t> domain_values(n_cells); | ||
std::ranges::fill(domain_values, domain_value); | ||
|
||
std::vector<std::int32_t> material_values(n_cells); | ||
std::ranges::fill(material_values, material_value); | ||
|
||
mesh::MeshTags<std::int32_t> mt_domains(mesh->topology(), 2, indices, | ||
domain_values); | ||
mt_domains.name = "domain"; | ||
|
||
mesh::MeshTags<std::int32_t> mt_materials(mesh->topology(), 2, indices, | ||
material_values); | ||
mt_materials.name = "material"; | ||
|
||
io::XDMFFile file(mesh->comm(), mesh_file, "w", io::XDMFFile::Encoding::HDF5); | ||
file.write_mesh(*mesh); | ||
file.write_meshtags(mt_domains, mesh->geometry(), | ||
"/Xdmf/Domain/mesh/Geometry"); | ||
file.write_meshtags(mt_materials, mesh->geometry(), | ||
"/Xdmf/Domain/Grid/Geometry"); | ||
|
||
file.close(); | ||
|
||
io::XDMFFile meshFile(MPI_COMM_WORLD, mesh_file, "r", | ||
io::XDMFFile::Encoding::HDF5); | ||
mesh = std::make_shared<mesh::Mesh<double>>(meshFile.read_mesh( | ||
fem::CoordinateElement<double>(mesh::CellType::triangle, 1), | ||
mesh::GhostMode::none, "mesh")); | ||
|
||
const auto mt_domain = meshFile.read_meshtags_by_name( | ||
*mesh, "domain", "domain", "/Xdmf/Domain"); | ||
|
||
CHECK(mt_domain.values().front() == domain_value); | ||
|
||
const auto mt_material = meshFile.read_meshtags_by_name( | ||
*mesh, "material", "material", "/Xdmf/Domain"); | ||
|
||
CHECK(mt_material.values().front() == material_value); | ||
|
||
CHECK_THROWS(meshFile.read_meshtags_by_name(*mesh, "mesh", "missing")); | ||
} | ||
|
||
} // namespace | ||
|
||
TEST_CASE("Read meshtag by name", "[read_meshtag_by_name]") | ||
{ | ||
CHECK_NOTHROW(test_read_named_meshtags()); | ||
} |
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.
Let's change
name
->label
in the function name. 'name' is ambiguous.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.
Done