Correct invalid names in journal files#2086
Conversation
and minor clean-up.
There was a problem hiding this comment.
Pull request overview
This PR updates tutorial Cubit/Exodus workflows to avoid invalid (and potentially truncated) entity names by switching tutorials from ID-based condition assignment to node-set-name-based assignment, and adds a guard in the core condition parser to enforce the 32-character Exodus name limit.
Changes:
- Rename nodeset names in FSI tutorial journal files to remove spaces and update corresponding
.4C.yamlfiles to useNODE_SET_NAMEinstead ofnode_set_id. - Update the battery tutorial journal and input YAML to use new (shorter) nodeset names and refreshed tutorial/baseline settings.
- Add a
NODE_SET_NAMElength check (≤ 32 chars) inCore::Conditions::ConditionDefinition::read()and tighten the API withconstinput and[[nodiscard]]getters.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tutorials/fsi/tutorial_fsi_3d.jou | Renames nodeset names (removes spaces) for Cubit validity and stable lookup. |
| tests/tutorials/fsi/tutorial_fsi_3d.4C.yaml | Switches multiple conditions to reference nodesets by NODE_SET_NAME. |
| tests/tutorials/fsi/tutorial_fsi_2d.jou | Renames nodeset names and adjusts exported mesh filename. |
| tests/tutorials/fsi/tutorial_fsi_2d.4C.yaml | Switches conditions to NODE_SET_NAME and aligns mesh file reference. |
| tests/tutorials/battery/tutorial_battery.jou | Updates naming and (additionally) changes a number of geometry/meshing commands. |
| tests/tutorials/battery/tutorial_battery.4C.yaml | Updates to NODE_SET_NAME plus broader solver/BC/baseline changes. |
| src/core/fem/src/condition/4C_fem_condition_definition.hpp | Makes condition parsing API more const-correct and marks getters [[nodiscard]]. |
| src/core/fem/src/condition/4C_fem_condition_definition.cpp | Enforces 32-char limit for NODE_SET_NAME and updates parsing signature. |
|
As Copilot noted. Along the way, I also adapted the battery tutorial a bit. ;-) |
rjoussen
left a comment
There was a problem hiding this comment.
Changes look good to me! I left one suggestion.
ed52eb6 to
ce4676d
Compare
| if (parsed_condition_data.node_set_name.has_value()) | ||
| { | ||
| FOUR_C_ASSERT_ALWAYS(parsed_condition_data.node_set_name.value().size() <= 32, | ||
| "NODE_SET_NAME '{}' exceeds the maximum length of 32 characters.", | ||
| parsed_condition_data.node_set_name.value()); | ||
| } | ||
|
|
There was a problem hiding this comment.
If I understand this correctly, this now always throws an error if there is a condition definition in the input with a name longer than 32 characters. Isn't this overly restrictive? Gmsh and vtu meshes have no problem with reading meshes with long node set names. Maybe we shouldn't check that here, but in the Exodus reader itself, when a nodeset was read that has exactly 32 characters? Or, we only display a note about this problem when a nodeset has not been found by name, and it has more than 32 characters.
There was a problem hiding this comment.
I also thought about that, but the problem is that I do not have the relevant information in the Exodus reader. One could only use the name length provided by the Exodus method as a proxy. If it has fewer than 32 characters, it was ok; if it has 32 characters, it might have been exactly the right length or too long. Which also felt like an unsatisfying solution to me.
However, you are right that this may be too strict for other mesh readers (I do not know). I just thought that 32 characters might be long enough anyway. But if you have doubts, we could only output a warning stating that for the Exodus reader, 32 characters is the maximum, and then the code would terminate later anyway, as the condition is not found in the mesh.
There was a problem hiding this comment.
we could only output a warning stating that for the Exodus reader, 32 characters is the maximum, and then the code would terminate later anyway, as the condition is not found in the mesh.
I would be in favor of that. This would only look a little strange if someone defines a name in exodus with exactly 32 characters and already knows about the limitation. In almost all scenarios, the user would see a warning about the length first and a crash immediately afterwards.
Description and Context
As reported in #1973, we have journal files with invalid names in the repo. This issue is addressed by changing the names in the journal files to valid names (in Coreform Cubit terms) and assigning the conditions using this name.
While changing this, I realized that we can only have 32-character names, as the Exodus API for reading the mesh truncates the name beyond 32 characters, which resulted in 4C not finding the corresponding nodeset. Thus, the first commit adds a check to ensure that the name is not longer than 32 characters. The others are changing the affected tutorials test by test.
Related Issues and Pull Requests
Closes #1973