-
Notifications
You must be signed in to change notification settings - Fork 252
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
[MedApp] Trim Whitespace in Group Names #12147
Conversation
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.
so it is an actual space and not a null-char \0
?
I would say remove all leading and trailing spaces and other crap 😅
Perhaps add a warning once, otherwise this might be annoying to debug
r_smp_name.erase(r_smp_name.begin(), | ||
std::find_if(r_smp_name.begin(), | ||
r_smp_name.end(), | ||
[](std::string::value_type c) {return !std::isspace(c);})); |
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.
other languages: foo.ltrim()
And then there is C++ 🤦
for (const auto& r_smp_name : r_map.second) { | ||
for (auto& r_map : groups_by_fam) { | ||
for (auto& r_smp_name : r_map.second) { | ||
// Trim whitespace from group names |
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.
maybe better to do this right in RemoveTrailingNullChars
?
BTW I remembered this function: Kratos/kratos/sources/model_part_io.cpp Lines 5110 to 5113 in 24f6664
Maybe you could use sth similar? (ofc should be non-const 😆) |
good idea! I got tangled up in several other PRs, but hopefully I can get back to this by Friday morning. |
turns out std::isspace already includes all whitespace characters. |
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.
Almost!
for (const auto& r_smp_name : r_map.second) { | ||
for (auto& r_map : groups_by_fam) { | ||
for (auto& r_smp_name : r_map.second) { | ||
RemovePadding(r_smp_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.
I dont think this is still necessary, since the group names are already cleaned in L349 (see above)
bool IsNotPaddingCharacter(std::string::value_type character) noexcept | ||
{ | ||
return !(std::isspace(character) || character == '\0'); | ||
} | ||
|
||
// The names in the MED-file often have trailing null-chars, which need to be removed | ||
// this can otherwise make debugging very tricky | ||
void RemoveTrailingNullChars(std::string& rInput) | ||
void RemovePadding(std::string& rInput) | ||
{ | ||
// Trime left | ||
rInput.erase(rInput.begin(), | ||
std::find_if(rInput.begin(), | ||
rInput.end(), | ||
IsNotPaddingCharacter)); | ||
|
||
// Trim right | ||
rInput.erase(std::find_if(rInput.rbegin(), | ||
rInput.rend(), | ||
IsNotPaddingCharacter).base(), | ||
rInput.end()); | ||
rInput.erase(std::find(rInput.begin(), rInput.end(), '\0'), rInput.end()); | ||
} |
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.
hmm this looks now like a pretty good addition to the StringUtilities
Could you please add it? I can review fast ;)
Not a blocker, we can also merge this one first.
Ideally this should have tests, but would not fit the current structure of the code, but once it is in the StringUtils
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.
see #12225
@matekelemen now that #12225 is merged, can you please update this PR accordingly? |
Better late than never. |
auto groups_by_fam = GetGroupsByFamily( | ||
mpFileHandler->GetFileHandle(), | ||
mpFileHandler->GetMeshName()); | ||
|
||
// create SubModelPart hierarchy | ||
for (const auto& r_map : groups_by_fam) { | ||
for (const auto& r_smp_name : r_map.second) { | ||
for (auto& r_map : groups_by_fam) { | ||
for (auto& r_smp_name : r_map.second) { |
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.
why removing the const
?
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.
Leftovers ...
I fixed them now.
@@ -498,7 +491,7 @@ void MedModelPartIO::ReadModelPart(ModelPart& rThisModelPart) | |||
const int dimension = mpFileHandler->GetDimension(); | |||
|
|||
// read family info => Map from family number to group names aka SubModelPart names | |||
const auto groups_by_fam = GetGroupsByFamily( |
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.
and this one?
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.
got it
Salome sometimes (for example during uniform mesh refinement) pads group names with whitespace to fill its group name limit (64 characters), which is extremely annoying. Trimming the group names recovers the original names set in the editor, but creates a discrepancy between group names in the MED files and its corresponding model parts.
@philbucher this is just a proposal; what do you think? Should I rather modify the padded MED files instead of processing them during reading?