Skip to content

[MedApplication] Preserve node and geometry IDs in MED I/O with SubModelParts - #14201

Merged
Marco1410 merged 17 commits into
masterfrom
marco/med_application
Apr 8, 2026
Merged

[MedApplication] Preserve node and geometry IDs in MED I/O with SubModelParts #14201
Marco1410 merged 17 commits into
masterfrom
marco/med_application

Conversation

@Marco1410

Copy link
Copy Markdown
Contributor

📝 Description
This PR updates the MED I/O so that Kratos can write and read MED files preserving node and geometry IDs, including SubModelParts.

Main points:

  • Node IDs are preserved during write/read.
  • Geometry IDs are preserved.
  • SubModelParts in Kratos are saved as family groups in Salome and correctly reconstructed when reading.
  • Proper handling of SALOME points vs Kratos nodes to avoid creating unwanted point geometries.

I tested this with Python and C++ tests, and also with HROM potential flow models. So far everything works as expected.

🆕 Changelog

  • Updated 'med_model_part_io.cpp' (WriteModelPart and ReadModelPart)
  • Added/updated Python and C++ tests

@philbucher philbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

I have some concerns, but overall looks good

I leave approval to current users

for (int i = 0; i < num_groups; i++) {
group_names[i] = StringUtilities::Trim(c_group_names.substr(i * MED_LNAME_SIZE, MED_LNAME_SIZE), /*RemoveNullChar=*/true);
std::string raw_name( c_group_names.data() + i * MED_LNAME_SIZE, MED_LNAME_SIZE);
raw_name = StringUtilities::Trim(raw_name, true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this change a bit? I thought the nullchars are already handled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments Philipp.

With the original version, the group names contain garbage characters at the end. To fix this, I trim the strings at the "\0" null terminator.

At least on Ubuntu, this works.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trim the strings at the "\0" null terminator.

StringUtilities::Trim should already be doing this. What is the failing input?
Could you please add the failing input in this test so that it could be fixed there?

reorder_fct(geom_node_ids);

// Avoid using nodes or points as geometries
if (geo_type == MED_POINT1) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are those skipped? Structural cases need them! (e.g. pointloads)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skipping the creation of point geometries, as they don't represent real geometries in Kratos.

Here, node families are used to construct SubModelParts that contain only nodes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they should represent real geometries. If you dont want them then you shouldnt create them in Salome.

See an example for a simple cantilever how they are used:
https://github.com/KratosMultiphysics/KratosSalomePlugin/blob/dce31197e15887b40174d27783ac2839b0134a2e/tui_examples/cantilever/salome_cantilever_hexa.py#L60

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

num_nodes,
dimension);

// get global numbering for nodes, if the file contains them

@philbucher philbucher Feb 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah man, despite my best efforts the IDs are always finding their way back :D

Main reason I am against using ids is that they are often (ab) used to get nodes from ModelParts. This is a search, which is expensive for large models
=> one should use position in the ModelPart instead with is O(1)
Also non-consecutive IDs can create problems for MPI-partitioning

personal rant aside, this should be optional, as some meshers dont even write the IDs
I would vote to not read the IDs by default, but that decision I leave to current users or the tech-committee

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s true, but in any case, if we do nothing, the MED library enumerates them in its own way. This option is available in both Kratos and Salome, so for now, I think it is better to keep them synchronised. We can remove this later if needed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well until now the Ids were just enuerated from the node position

In any case, I dont have a strong opinion

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's optional now, but locally just so we don't forget we're doing it until someone says something about it.

Comment thread applications/MedApplication/custom_utilities/med_testing_utilities.cpp Outdated
<< "Using MED implicit numbering." << std::endl;

for (med_int i = 0; i < num_nodes; ++i) {
node_ids[i] = i + 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use std::iota or sth similar

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@philbucher philbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered some more potential conceptual issues with the export of SubModelParts

Comment on lines +752 to +759
med_int dimension = 0;
for (const auto& r_geom : rThisModelPart.Geometries()) {

dimension = std::max( dimension, static_cast<med_int>(r_geom.WorkingSpaceDimension()));
}
if (dimension == 0 || dimension == 1){
dimension = 3;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
med_int dimension = 0;
for (const auto& r_geom : rThisModelPart.Geometries()) {
dimension = std::max( dimension, static_cast<med_int>(r_geom.WorkingSpaceDimension()));
}
if (dimension == 0 || dimension == 1){
dimension = 3;
}
med_int dimension = 2;
for (const auto& r_geom : rThisModelPart.Geometries()) {
dimension = std::max( dimension, static_cast<med_int>(r_geom.WorkingSpaceDimension()));
}

CheckMEDErrorCode(err, "MEDmeshGlobalNumberWr (nodes)");

med_int next_family = 1;
std::unordered_map<std::string, med_int> smp_to_family;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered part of why I didnt implement the writing of SubModelParts: A family in med is NOT the same as a SubModelPart in Kratos. A family contains only one type of geometry (e.g. only Triangle3 or Tetra4).
The med equivalent of a SubModelPart is a group, which consists of multiple families. (actually the relation is the other way round, each family has groups)

Also what I think is not considered is if a geometry belongs to multiple SubModelParts.

Back then I thought the solution would be to tag each entity like is done for the MMG-stuff, but not sure


CheckMEDErrorCode(err, "MEDmeshGlobalNumberWr (nodes)");

med_int next_family = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI by definition node-families are positive, whereas geometry families are negative

CheckMEDErrorCode(err, "MEDmeshGlobalNumberWr (cells)");

const std::size_t nb_elem = conn.size();
std::vector<med_int> elem_family_numbers(nb_elem, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::vector<med_int> elem_family_numbers(nb_elem, 0);
std::vector<med_int> geom_family_numbers(nb_elem, 0);


std::function<void(const ModelPart&)> collect_subparts =
[&](const ModelPart& mp) {
for (const auto& r_child : mp.SubModelParts()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

med doesnt know SubSubModelParts, I am curious how you handle it. Have you tested it?

@Marco1410

Copy link
Copy Markdown
Contributor Author

Hi @philbucher!!, I’ve made some changes to support families and groups, and to use the hierarchical naming of model parts so it works with submodelparts and nested submodelparts.

I’ve been testing it, and so far everything works as expected.

Thanks for your feedback, and when you have time, I’d really appreciate it if you could take a look.

@Marco1410
Marco1410 requested a review from philbucher March 24, 2026 12:59

@philbucher philbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok from my side 👍
I didnt review the algorithm in detail but I trust your tests

Approval I leave to some current users

for (int i = 0; i < num_groups; i++) {
group_names[i] = StringUtilities::Trim(c_group_names.substr(i * MED_LNAME_SIZE, MED_LNAME_SIZE), /*RemoveNullChar=*/true);
std::string raw_name( c_group_names.data() + i * MED_LNAME_SIZE, MED_LNAME_SIZE);
raw_name = StringUtilities::Trim(raw_name, true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trim the strings at the "\0" null terminator.

StringUtilities::Trim should already be doing this. What is the failing input?
Could you please add the failing input in this test so that it could be fixed there?

BuiltinTimer timer;

const bool add_nodes_of_geometries = true; // TODO make this an input parameter
const bool preserve_global_numbering_for_nodes = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is a behavior change some current user should approve this PR
@sunethwarna @matekelemen


std::vector<med_int> node_ids(num_nodes);
// get global numbering for nodes, if the file contains them
if (preserve_global_numbering_for_nodes) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like how its handled for the geometry
If the file contains the IDs then for me it would be fine to use them. If not issue a warning and do whats currently done

=> then you wouldnt need the preserve_global_numbering_for_nodes flag


KratosMed.MedTestingUtilities.CheckModelPartsAreEqual(
self.mp_read_1, self.mp_read_2, check_sub_model_parts=False # until writing of SMPs is implemented
self.mp_read_1, self.mp_read_2, check_sub_model_parts=True # until writing of SMPs is implemented

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.mp_read_1, self.mp_read_2, check_sub_model_parts=True # until writing of SMPs is implemented
self.mp_read_1, self.mp_read_2, check_sub_model_parts=True

now its implemented no?

@Marco1410

Copy link
Copy Markdown
Contributor Author

@rubenzorrilla I've added the last changes.

Comment thread applications/MedApplication/custom_io/med_model_part_io.cpp Outdated

@philbucher philbucher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving since the new behavior is optional wrt the numbering

Thanks!

@Marco1410
Marco1410 merged commit 3f551a2 into master Apr 8, 2026
10 checks passed
@Marco1410
Marco1410 deleted the marco/med_application branch April 8, 2026 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants