-
-
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?
Conversation
python/dolfinx/io/utils.py
Outdated
return MeshTags(mt) | ||
|
||
def read_meshtags(self, mesh, name, xpath="/Xdmf/Domain"): |
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.
Needs docs.
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
python/dolfinx/io/utils.py
Outdated
@@ -276,10 +276,13 @@ def read_mesh( | |||
) | |||
return Mesh(msh, domain) | |||
|
|||
def read_meshtags(self, mesh, name, attribute_name="", xpath="/Xdmf/Domain"): | |||
mt = super().read_meshtags(mesh._cpp_object, name, attribute_name, xpath) | |||
def read_meshtags_by_name(self, mesh, name, attribute_name, xpath="/Xdmf/Domain"): |
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.
Would it be better as one function and using default arg attribute_name=None
?
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 remember that you don't like having two default arguments in one function but I'm guessing that's only for C++, right?
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.
Not good for C++, fine with Python if it's simple (which this is) and doesn't introduce complicated conditional handling.
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
const mesh::Mesh<double>& mesh, const std::string name, | ||
const std::string attribute_name, const std::string xpath) | ||
mesh::MeshTags<std::int32_t> | ||
XDMFFile::read_meshtags_by_name(const mesh::Mesh<double>& mesh, |
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
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.
The code is XDMFFile.cpp
looks to me more complicated than it needs to be. I've left some suggestions as a first pass. I can look again once it's been simplified some more.
cpp/dolfinx/io/XDMFFile.h
Outdated
@@ -164,6 +164,16 @@ class XDMFFile | |||
const mesh::Geometry<T>& x, std::string geometry_xpath, | |||
std::string xpath = "/Xdmf/Domain"); | |||
|
|||
/// Read MeshTags by name | |||
/// @param[in] mesh The Mesh that the data is defined on | |||
/// @param[in] name |
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.
What is name
? Can you add a doctstring?
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, but for the record this was already undocumented.
cpp/dolfinx/io/XDMFFile.cpp
Outdated
if (not attribute_label.empty()) | ||
{ | ||
bool found = false; | ||
for (; attribute_node; |
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.
This looks weird and is hard to read. Should it be while
loop?
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
cpp/dolfinx/io/XDMFFile.cpp
Outdated
{ | ||
pugi::xml_attribute hint; | ||
pugi::xml_attribute name = attribute_node.attribute("Name", hint); | ||
if (std::string(name.value()) == attribute_label) |
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.
Creating the string is a bit clumsy, maybe use https://en.cppreference.com/w/cpp/string/char_traits/compare.
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.
Using the function you suggested requires providing a number of characters to compare, so it would have to be something like
if (auto len = std::max(std::strlen(name.value()), attribute_label.size());
std::char_traits<char>::compare(name.value(), attribute_label.c_str(),
len) == 0)
right?
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 think name.value() == attribute_label
might work, see https://en.cppreference.com/w/cpp/string/basic_string/operator_cmp.
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!
for (; attribute_node; | ||
attribute_node = attribute_node.next_sibling("Attribute")) | ||
{ | ||
pugi::xml_attribute hint; |
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.
What is this for?
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.
It's something that pugixml
requires, like a hint to try to speed-up the search of a node.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can because hint
is also an output parameter of the function.
cpp/test/mesh/Domain.xdmf
Outdated
@@ -0,0 +1,26 @@ | |||
<Xdmf Version="3.0"> |
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.
Can we avoid adding data files? Can it be created by the test rather than added to the repo?
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 can convert the file to ascii, add it to the source code and have the code write it to /dev/shm
, then read it back. How about that? :)
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.
On second though, that's really just the same thing as adding a file with the ascii content, which is what I do now XD
Please advise!
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.
Ok, never mind... I turned on my brain and found a solution.
…nx into mleoni/readNamedMeshtags
@@ -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 comment
The 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 comment
The 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 name
parameter which refers to the name of the mesh node in the same file. I picked the word "label" to distinguish the two internally, in the code implementation, but a user should not be concerned with this implementation detail: a user saved a mesh tag giving it a "name" and wants to read it back by the same "name".
The fact remains that the user still needs to call a function whose name includes the word "label", not "name", which is indeed a bit confusing. I will change everything to be called more consistently, the code shouldn't become too unintelligible on the development side.
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.
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...
for (; attribute_node; | ||
attribute_node = attribute_node.next_sibling("Attribute")) | ||
{ | ||
pugi::xml_attribute hint; |
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.
The hint is not set or re-used - does the code compile without it?
if (!attribute_label.empty()) | ||
{ | ||
bool found = false; | ||
while (!found and attribute_node) |
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.
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 comment
The 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 "Name"
and the "basic functionality" functions do not allow to search for a name. If I wanted to loop using next_attribute
I would then need to check if the attribute's name is "Name"
and, if not, continue
.
I will add documentation but the code doesn't look to exotic to me [because I wrote it, of course XD].
cpp/dolfinx/io/XDMFFile.cpp
Outdated
{ | ||
pugi::xml_attribute hint; | ||
pugi::xml_attribute name = attribute_node.attribute("Name", hint); | ||
if (std::string(name.value()) == attribute_label) |
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 think name.value() == attribute_label
might work, see https://en.cppreference.com/w/cpp/string/basic_string/operator_cmp.
If no
attribute_name
is provided, read the first attribute. Otherwise, read the attribute with the provided name or throw an error if it couldn't be found.