[Core] Geometries to entities modeler

[rubenzorrilla]

📝 Description
This PR adds a first implementation of a modeler to create elements and conditions from a given list of modelparts containing geometries. I say first implementation because it is not parallel yet, neither in OpenMP and MPI. It cannot be parallel in OpenMP due to current limitations of the geometry container aka PointerHashMapSet (I reached this conclusion together with @pooyan-dadvand). I leave the MPI implementation for the future as its implementation is much more involving (most probably we will require renumbering the temporary local ids by doing a scattering).

@jginternational Just for you to know, this will be used in combination with the geometry-based input you're working on.
@mcgicjn2 @huhaas @avdg81 The idea is that you can directly use this within your future c++ multistage framework.

[loumalouomega]

We should follow #10726 (and this could be approved as we have been considering in our side during months)

FYI @pooyan-dadvand

[philbucher]

very nice, this fits well also my needs :)

[rubenzorrilla]

We should follow #10726 (and this could be approved as we have been considering in our side during months)

FYI @pooyan-dadvand

Note that your proposal in #10726 is a Process while this is a Modeler. This is determinant for the order of execution (another process might require stuff previously done by a modeler). Besides, multistage preprocess doesn't allow the execution of processes, so #10726 doesn't serve in that context. Last but not least, this supports multiple modelparts, while #10726, as far as I see, supports only one.

On top of that, this is a key feature for the future geometry-based GiD I/O, which is crucial for a coming soon deadline in our side. Besides this, I'd like to recall this in MeshIO repository. This modeler will be required for that as well.

Finally, I'd like to say that blocking a new completely transversal and key feature that benefits the whole project because another thing (which has been admittedly stucked for quite a lot) is used in your side is, in my opinion, not a sufficient technical reason. Of course this is a personal opinion.

EDIT: some typos.

[loumalouomega]

We should follow #10726 (and this could be approved as we have been considering in our side during months)
FYI @pooyan-dadvand

Note that your proposal in #10726 is a Process while this is a Modeler. This is determinant for the order of execution (another process might require stuff previously done by a modeler). Besides, multistage preprocess doesn't allow the execution of processes, so #10726 doesn't serve in that context. Last but not least, this supports multiple modelparts, while #10726, as far as I see, supports only one.

I know that a process and a modeler are not the same, I mean the changes in syntax proposed in that PR:

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element2D3N;Element2D4N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

or :

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element#D#N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

In order to be able to replace more than one type of element in the model part (you may have different types of geometries).

On top of that, this is a key feature for the future geometry-based GiD I/O, which is crucial for a coming soon deadline in our side. Besides this, I'd like to recall this in MeshIO repository. This modeler will be required for that as well.

I know, it was me who give support to geometries to the ModelPartIO as an intermediate step. In fact, I was going to do a modeler following design of #10726 after #10726 was merged, but that never happened.

Finally, I'd like to say that blocking a new completely transversal and key feature that benefits the whole project because another thing (which has been admittedly stucked for quite a lot) is used in your side is, in my opinion, not a sufficient technical reason. Of course this is a personal opinion.

It would be nice if that PR is reviewed, it is from 3th Febrary, no technical reason has been provided to actually block it, and in @KratosMultiphysics/altair we have already replaced all our internal processes following that design.

[rubenzorrilla]

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element2D3N;Element2D4N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

or :

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element#D#N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

In order to be able to replace more than one type of element in the model part (you may have different types of geometries).

This is exactly what we want to avoid. In other words, one submodelpart one type of entity. If the same model part is to contain different types of elements from different types of geometries (e.g. the structure) the I/O needs to write them in separate submodelparts.

[loumalouomega]

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element2D3N;Element2D4N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

or :

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element#D#N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

In order to be able to replace more than one type of element in the model part (you may have different types of geometries).

This is exactly what we want to avoid. In other words, one submodelpart one type of entity. If the same model part is to contain different types of elements from different types of geometries (e.g. the structure) the I/O needs to write them in separate submodelparts.

Not necessarily, you may combine tetra with hexa using pyramids as transition. I don't agree in this point.

[rubenzorrilla]

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element2D3N;Element2D4N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

or :

// Compute process
Parameters settings( R"(
{
    "element_name"   : "Element#D#N",
    "condition_name" : "LineCondition2D2N"
}  )" );

ReplaceElementsAndConditionsProcess process(r_model_part, settings);
process.Execute();

In order to be able to replace more than one type of element in the model part (you may have different types of geometries).

This is exactly what we want to avoid. In other words, one submodelpart one type of entity. If the same model part is to contain different types of elements from different types of geometries (e.g. the structure) the I/O needs to write them in separate submodelparts.

Not necessarily, you may combine tetra with hexa using pyramids as transition. I don't agree in this point.

Then the I/O will write the tetra geometries in a submodelpart and the pyramids in another one, so you'll end up doing

"elements_list" : [{
       "model_part_name" : "FluidSubModelPart.Tetrahedras",
       "element_name" : "QSVMS3D4N"
},{
       "model_part_name" : "FluidSubModelPart.Pyramids",
       "element_name" : "QSVMS3D5N"
}

I think that, on top of resulting in a much simpler input/output and implementation, having the geometries separated in submodelparts since the very beginning might be handy in the future.

[loumalouomega]

But my proposal doesn't break your design, you use it the same way if you want, but you have the option of not. Think that if you create a mesh with GMSH mixing tetras and hexas, gmsh will not differentiate the bodies, because doesn't work that way. This design will give you fewer problems and give you more flexibility.

[rubenzorrilla]

@loumalouomega I've been discussing this with @roigcarlo. We have considered the use case you told me this morning (i.e. having a unique modelpart with several geometries instead of the submodelpart division we are assuming in here). Revisiting #10726 we think that rather than introducing more complexity in here, it could be better to do it in several steps, which will result in two different utilities (note that this is what I suggested in here. In this regard I note that @roigcarlo reached this same conclusion without any knowledge of it.

Hence, our suggestion would be to

1. Classify the geometries in different modelparts / submodelparts. This would be done by an independent modeler, namely AssignGeometriesToModelPart or something of this sort. We note that in here we may reuse some of the stuff you implemented in [Core] Improve ReplaceElementsAndConditionsProcess #10726. Long story short, in your case you'll be using your stuff and in ours we may directly read what our I/O writes.
   Important: At this point we have a common intermediate representation of input geometries
2. At this point, we'll have the separation required by the modeler we suggest in here.

Nonetheless, we both think that before going further on this it would be better to reach a consensus in the @KratosMultiphysics/technical-committee .

[philbucher]

probably a dataclass would be better her. That one you can make frozen!

from dataclasses import dataclass

@dataclass(frozen=True)
class CppFunctionNames:
    Modelers: str = "GetModeler"
    Operations: str = "GetOperation"
    Processes: str = "GetProcesse"

this is even type safe, maybe works better for you

[rubenzorrilla]

Nice. I'll add it in a follow up PR (just in case).

[roigcarlo]

Why is this a warning? Isn't this why the function was made for? (Maybe should be KRATOS_INFO_IF).
Imho warning should be if n_elements == 0.

Also you could merge both functions (see LoopEntitiesList)

[rubenzorrilla]

The idea is to inform the user that there are elements (entities) to be removed.

[roigcarlo]

same

[rubenzorrilla]

Same answer.

[pooyan-dadvand]

Why do you wipe the modelpart before adding entities?

[rubenzorrilla]

Why do you wipe the modelpart before adding entities?

I discussed this with @RiccardoRossi and we both believe that it is much safer in this way. If this becomes in a limitation in the future we can easily remove it. Note that doing it in the other way around could easily turn in a pain in the neck (or even impossible...).

[pooyan-dadvand]

We usually use the modelers in a list of modelers to have an accumulative effect. This modeler don't follow this idea. And, if we want to change this afterward, It will be a change the behavior and very difficult to be accepted....

[rubenzorrilla]

We usually use the modelers in a list of modelers to have an accumulative effect. This modeler don't follow this idea. And, if we want to change this afterward, It will be a change the behavior and very difficult to be accepted....

Let me answer by points for the sake of clarity

• Well, this can be perfectly used in an accumulative manner (indeed we're already doing so for the fluid input). Note that erasing the already existing elements/conditions doesn't preclude using this in a sequence of modellers. It is only the behavior of this modeler.
• Note that not wiping elements/conditions might be quite tricky, specially for beginners, since you can easily end up with accidentally duplicated elements without realizing if your mdpa has both geometries and entities. If not removed by the modeler, you'd require the user to first add a remove entities modeler, something that would be in my opinion not straightforward for rookies and unnecessarily verbose.
• If you are considering the case of mixing entities input from the mdpa and the modeler (i.e. some elements are created when parsing the mdpa and some of them by the modeler) I don't think it is the way to go. In the long run, we want to go for the geometry-based I/O, which is what we can actually ask to any mesher. Allowing "mixed" input would in my opinion be against this. Note that you can apply the modeler to the same model part as many times as needed to create different elements from the same geometry (e.g. what we do for a buoyancy solver).
• I also note that changing the behavior from removing to not removing is much easier than doing it in the other way around.

In any case, if this is a blocker for you, we can easily make the wiping optional by adding a bool flag defaulted to true to the settings.

EDIT: A typo.

[philbucher]

Are you sure you want to allow mdpas with geometries AND elements/conditions?

I would prevent that, I think one should be forced to either use the old way (reading elements/conditions) OR the new way (reading geometries)

[ddiezrod]

• If you are considering the case of mixing entities input from the mdpa and the modeler (i.e. some elements are created when parsing the mdpa and some of them by the modeler) I don't think it is the way to go. In the long run, we want to go for the geometry-based I/O, which is what we can actually ask to any mesher. Allowing "mixed" input would in my opinion be against this. Note that you can apply the modeler to the same model part as many times as needed to create different elements from the same geometry (e.g. what we do for a buoyancy solver).

Just to say that we are already doing this in @KratosMultiphysics/altair side for Additive Manufacturing parts, where the part itself is meshed with tets and comes from mdpa, but the supports, which are complex geometries, are meshed with a voxel mesher that is inside our solver (and hopefull soon in public side too). It kind of worries me that we are going against that kind of solution here...

[rubenzorrilla]

• If you are considering the case of mixing entities input from the mdpa and the modeler (i.e. some elements are created when parsing the mdpa and some of them by the modeler) I don't think it is the way to go. In the long run, we want to go for the geometry-based I/O, which is what we can actually ask to any mesher. Allowing "mixed" input would in my opinion be against this. Note that you can apply the modeler to the same model part as many times as needed to create different elements from the same geometry (e.g. what we do for a buoyancy solver).

Just to say that we are already doing this in @KratosMultiphysics/altair side for Additive Manufacturing parts, where the part itself is meshed with tets and comes from mdpa, but the supports, which are complex geometries, are meshed with a voxel mesher that is inside our solver (and hopefull soon in public side too). It kind of worries me that we are going against that kind of solution here...

Well, having the mesher within the solver is in my opinion an extremely intrusive solution that shouldn't be considered as the rule for the general repo. Having said this, and without knowing if it is possible, my suggestion would be to deal with the supports in a first "fake" stage to then solve the physics in a second one. Alternatively, you may slightly change the writing to write geometries where you are now writing elements, then have a call to a MeshSupportsModeler that calls your voxel mesher and finally call the modeler in this PR to turn all geometries to entities.

Finally, I'd like to add that this might differ with the current solution in your repo but the point is to be as generic as possible (I recall here the discussion we had with MeshIO maintainers about the limitations of element/condition - based input). Last but not least, this PR only adds a new feature which shouldn't affect any of your current workflows.

[RiccardoRossi]

• If you are considering the case of mixing entities input from the mdpa and the modeler (i.e. some elements are created when parsing the mdpa and some of them by the modeler) I don't think it is the way to go. In the long run, we want to go for the geometry-based I/O, which is what we can actually ask to any mesher. Allowing "mixed" input would in my opinion be against this. Note that you can apply the modeler to the same model part as many times as needed to create different elements from the same geometry (e.g. what we do for a buoyancy solver).

Just to say that we are already doing this in @KratosMultiphysics/altair side for Additive Manufacturing parts, where the part itself is meshed with tets and comes from mdpa, but the supports, which are complex geometries, are meshed with a voxel mesher that is inside our solver (and hopefull soon in public side too). It kind of worries me that we are going against that kind of solution here...

Well, having the mesher within the solver is in my opinion an extremely intrusive solution that shouldn't be considered as the rule for the general repo. Having said this, and without knowing if it is possible, my suggestion would be to deal with the supports in a first "fake" stage to then solve the physics in a second one. Alternatively, you may slightly change the writing to write geometries where you are now writing elements, then have a call to a MeshSupportsModeler that calls your voxel mesher and finally call the modeler in this PR to turn all geometries to entities.

Finally, I'd like to add that this might differ with the current solution in your repo but the point is to be as generic as possible (I recall here the discussion we had with MeshIO maintainers about the limitations of element/condition - based input). Last but not least, this PR only adds a new feature which shouldn't affect any of your current workflows.

just to tell that i agree with @rubenzorrilla 's arguments here...

[ddiezrod]

I understand @rubenzorrilla @RiccardoRossi , although I think that allowing the model part to already have some elements makes as much sense as not allowing it IMO. I just wanted to stress out that we are already doing many things with modelers that might require some flexibility. But it is fine, as you said, if we want to use this, we will find the way.

[pooyan-dadvand]

I don't agree with @rubenzorrilla comments. And definitely not with merging it without addressing our comments!

As @ddiezrod mentioned we have the use cases where we can have a partially created modelpart passed to the next modeler. This can become even more popular in multi stage scenario where you have elements and conditions from previous stage to get mapped or reused and you read a new mpda.

If the idea is to have this modeler only for your use cases at least rename it to a different name that says that it cleanups the modelpart. Then one can create the "generic" one.

[philbucher]

I wanted to use this in the regular workflow in the ProjectParameters, but I could not get it to work unfortunately, did not manage to import it

Can you please post a usage example for json? Thx

[RiccardoRossi]

To give my two cents, this feature was proposed already several months ago with one specific design objective:
enable reading only the geometries (without the elements) and filling a modelpart with new elements once this was done.
In a sense this is designed to be used once, just after reading in the geometries in the model_part_io.
Obviously the objective is to move some of the complexity from the preprocessor into the kratos. This is also an essential building block for our FlowGraph workflow which we are currently trying to push.

The assumpyion of the mp being empty at the beginning was taken from the day 1, and never put in discussion over several months. Meanwhile we put a lot of effort, in collaboration with @loumalouomega to adapt the input to the petition of Altair's team, which made an otherwise simple PR to last several months.

As of yesterday there were no technical issues aside for a disagreement of the name of the modeler (somethign on which we disagree) and the petition to ask for a flag to control initial erasal of elements, something that can be eventually done at a different PR. About this second point i do not think it is as easy as that ... but i'll expand on this later.
In any case, a different modeler, with a more flexible behaviour, can be easily added shoud anyone need it, so the current design is not preventing any future change.

In any case this was the last blocker for the point release which was generated yesterday, which was critically needed in one of our research projects, reason for which i consider perfectly justified to rush it in.

Having said this, i do believe that this is just an inital step. We definitely need a modeler to handling the passage between stages. In my opinion such a modeler will have to work with elements and conditions and not with geometries, defining what to do with element types, constitutive laws, internal variables etc. This will imply not just creating new elements but also removing or replacing existing ones. In any case my feeling is that it will not require the use of geometries as we are doing here.
One important point however is that my estimation is that such a modeler will require a much higher effort, since it will imply dealing with many corner cases. In case this feature becomes sufficiently advanced and easy to use to supersed the feature added in this PR, we can definitely start preferring it over the future.
As i understand such a feature is very important for Altair, who appears to have already some proposal about how that should be done.An important point however is who is going to put the necessary man power on this. My hope that the team at Altair is willing to contribute the necessary manpower (we will collaborate in the design of course, and make an effor to speed up convergence to an acceptable solution), as we currently do not have the bandwidth to prioritize this aspect as we did for the (much simpler) feature contained in current PR.

[loumalouomega]

To give my two cents, this feature was proposed already several months ago with one specific design objective: enable reading only the geometries (without the elements) and filling a modelpart with new elements once this was done. In a sense this is designed to be used once, just after reading in the geometries in the model_part_io. Obviously the objective is to move some of the complexity from the preprocessor into the kratos. This is also an essential building block for our FlowGraph workflow which we are currently trying to push.

The assumpyion of the mp being empty at the beginning was taken from the day 1, and never put in discussion over several months. Meanwhile we put a lot of effort, in collaboration with @loumalouomega to adapt the input to the petition of Altair's team, which made an otherwise simple PR to last several months.

As of yesterday there were no technical issues aside for a disagreement of the name of the modeler (somethign on which we disagree) and the petition to ask for a flag to control initial erasal of elements, something that can be eventually done at a different PR. About this second point i do not think it is as easy as that ... but i'll expand on this later. In any case, a different modeler, with a more flexible behaviour, can be easily added shoud anyone need it, so the current design is not preventing any future change.

In any case this was the last blocker for the point release which was generated yesterday, which was critically needed in one of our research projects, reason for which i consider perfectly justified to rush it in.

Having said this, i do believe that this is just an inital step. We definitely need a modeler to handling the passage between stages. In my opinion such a modeler will have to work with elements and conditions and not with geometries, defining what to do with element types, constitutive laws, internal variables etc. This will imply not just creating new elements but also removing or replacing existing ones. In any case my feeling is that it will not require the use of geometries as we are doing here. One important point however is that my estimation is that such a modeler will require a much higher effort, since it will imply dealing with many corner cases. In case this feature becomes sufficiently advanced and easy to use to supersed the feature added in this PR, we can definitely start preferring it over the future. As i understand such a feature is very important for Altair, who appears to have already some proposal about how that should be done.An important point however is who is going to put the necessary man power on this. My hope that the team at Altair is willing to contribute the necessary manpower (we will collaborate in the design of course, and make an effor to speed up convergence to an acceptable solution), as we currently do not have the bandwidth to prioritize this aspect as we did for the (much simpler) feature contained in current PR.

@RiccardoRossi, firstly, thank you for the detailed explanation on the feature's evolution and the intention behind it. It provides a lot of clarity on the context and the decisions made.

However, I'd like to offer a clarification on the timeline regarding my involvement. While I understand the desire to pinpoint the causes of delays, it's crucial to be accurate about it. The required changes from PR #11376 were addressed and merged in less than 8 days. In contrast, the overarching changes that were part of PR #10726 took almost six months to finalize. My specific block was resolved in under two weeks, and post that, any delays were due to reviews and changes stemming from other comments.

While I'm entirely committed to collaboration and ensuring the best for Kratos, I believe it's essential for the community to have an accurate understanding of the challenges and timelines associated with various PRs. Let's continue to work together, address issues collaboratively, and most importantly, ensure that any discussions or criticisms remain fact-based.

Thanks for your understanding.

[RiccardoRossi]

fair enough for your comment

[rubenzorrilla]

I wanted to use this in the regular workflow in the ProjectParameters, but I could not get it to work unfortunately, did not manage to import it

Can you please post a usage example for json? Thx

Sure. Here it is a working CFD example test_geometry_based_input_cfd.zip.

[pooyan-dadvand]

@RiccardoRossi First of all, despite being in CIMNE, Altair, TUM, TU-Braunschweig or Deltares, we are all here in Kratos team and try to make it better. Please don't forget that we drive Kratos by the needs of the community and all of us are involved in its development.

The assumpyion of the mp being empty at the beginning was taken from the day 1, and never put in discussion over several months. Meanwhile we put a lot of effort, in collaboration with @loumalouomega to adapt the input to the petition of Altair's team, which made an otherwise simple PR to last several months.

I didn't know if it was from day 1. I just found it when I went to review it.

As of yesterday there were no technical issues aside for a disagreement of the name of the modeler (somethign on which we disagree) and the petition to ask for a flag to control initial erasal of elements, something that can be eventually done at a different PR. About this second point i do not think it is as easy as that ... but i'll expand on this later.

• Removing the elements and conditions is a technical issue when I am reasoning it and is a common tool
• It wasn't renaming. In fact, I put the renaming as a workaround after it was merged to ease the future implementations.
• My argument is that this expansion will be a behavior change and difficult to introduce

In any case this was the last blocker for the point release which was generated yesterday, which was critically needed in one of our research projects, reason for which i consider perfectly justified to rush it in.

The argument of project is something that all we have and justification shall be argument correctly. At least, this comment would have helped before merging.

Having said this, i do believe that this is just an inital step. We definitely need a modeler to handling the passage between stages. In my opinion such a modeler will have to work with elements and conditions and not with geometries, defining what to do with element types, constitutive laws, internal variables etc. This will imply not just creating new elements but also removing or replacing existing ones. In any case my feeling is that it will not require the use of geometries as we are doing here. One important point however is that my estimation is that such a modeler will require a much higher effort, since it will imply dealing with many corner cases. In case this feature becomes sufficiently advanced and easy to use to supersed the feature added in this PR, we can definitely start preferring it over the future. As i understand such a feature is very important for Altair, who appears to have already some proposal about how that should be done.An important point however is who is going to put the necessary man power on this. My hope that the team at Altair is willing to contribute the necessary manpower (we will collaborate in the design of course, and make an effor to speed up convergence to an acceptable solution), as we currently do not have the bandwidth to prioritize this aspect as we did for the (much simpler) feature contained in current PR.

• I don't understand why you take our suggestion as Altair request. In fact, in Altair we don't have plans to change to geometries yet as it affects the GUI side. If both @ddiezrod and I are giving the same suggestion is due to our experience with several modelers and also in multi-stage solutions, which we want to transfer and make the common part more useful.
• About the manpower, as I said, it is not a priority in Altair. But we can help if is needed. Our focus now is on MPI search that I think will be a useful feature for the community.

Finally, I agree with @loumalouomega that we should move forward and keep planning and working together to avoid such situations.