Kratos
Kratos copied to clipboard
KratosMultiphysics
[MeshingApp] Tetrahedra3D10 refine utility
📝 Description Utility deriving form "Local_Refine_Tetrahedra_Mesh". Instead of dividing each tetrahedra3D4 of the mesh into other tetrahedra3D4, this utility adds new nodes between all the edges of every tetrahedra3D4 and then replaces it by a Tetrahedra3D10.
🆕 Changelog -New derived class "Tetrahedra10_mesh_converter_utility" -Corresponding tests
I don't know which is the need for a new class. I think that the same can be achieved by templating these methods in the base class and specializing them for each one of the geometries. Besides this is in my opinion a much clean design, it allows to automatically filter the function to be used. With this design, the user needs to specify which class to call.
Since this method is actually not refining the mesh but just replacing the element geometry by a more complex one, i don't see the use of a template. Templating would make sense to me if we were deciding whether to refine the mesh in one or another geometry type (maybe refining in Tetrahedra or in Triangles) , but i don't see here which class is to template, since the difference between my utility and base one is whether we divide or not, not the use of one or another geometry.
I think that the confusion comes from the "Refinement" in the name. This utility converts linear tetrahedra to quadratic ones and do not refine! So I would suggest @AriadnaCortesDanes to change the name of the utility.
@rubenzorrilla I am not sure how you want to achieve this by making it template. I think refactoring the base class and separate the part which adds the nodes from the one which creates the elements would be useful but I am not sure if @AriadnaCortesDanes can do this efficiently.
I think that the confusion comes from the "Refinement" in the name. This utility converts linear tetrahedra to quadratic ones and do not refine! So I would suggest @AriadnaCortesDanes to change the name of the utility.
@rubenzorrilla I am not sure how you want to achieve this by making it template. I think refactoring the base class and separate the part which adds the nodes from the one which creates the elements would be useful but I am not sure if @AriadnaCortesDanes can do this efficiently.
We can refactor current class so we have a private method with a template that takes the geometry type as argument. I think this would do the job. At the end, current design is only overriding two methods so it should be possible IMO.
I think that the confusion comes from the "Refinement" in the name. This utility converts linear tetrahedra to quadratic ones and do not refine! So I would suggest @AriadnaCortesDanes to change the name of the utility. @rubenzorrilla I am not sure how you want to achieve this by making it template. I think refactoring the base class and separate the part which adds the nodes from the one which creates the elements would be useful but I am not sure if @AriadnaCortesDanes can do this efficiently.
We can refactor current class so we have a private method with a template that takes the geometry type as argument. I think this would do the job. At the end, current design is only overriding two methods so it should be possible IMO.
I am not sure if I get your idea. This is not refining the tetrahedra. It just reuses the created nodes in the edges to covert it to the quadratic ones. The only overwritten method is EraseOldElementAndCreateNewElement. I think it is very confusing if a template refinement utility refines the mesh for one type and converting it for another type.
I think that the confusion comes from the "Refinement" in the name. This utility converts linear tetrahedra to quadratic ones and do not refine! So I would suggest @AriadnaCortesDanes to change the name of the utility. @rubenzorrilla I am not sure how you want to achieve this by making it template. I think refactoring the base class and separate the part which adds the nodes from the one which creates the elements would be useful but I am not sure if @AriadnaCortesDanes can do this efficiently.
We can refactor current class so we have a private method with a template that takes the geometry type as argument. I think this would do the job. At the end, current design is only overriding two methods so it should be possible IMO.
I am not sure if I get your idea. This is not refining the tetrahedra. It just reuses the created nodes in the edges to covert it to the quadratic ones. The only overwritten method is
EraseOldElementAndCreateNewElement. I think it is very confusing if a template refinement utility refines the mesh for one type and converting it for another type.
OK. Now I see your point. If this is the case, I think this new feature should be added as an option to current utility, rather than creating a new class.
Good work! I would suggest to move the implementation to the correponding cpp file to have less compile time dependency between files and minimize the compilation time.
Totally agree 👍
Good work! I would suggest to move the implementation to the correponding cpp file to have less compile time dependency between files and minimize the compilation time.
I tried to move the implementation but, as the methods are template, they needed explicit instantiation, which doesn't convince me. Personally, I would leave it this way.
I tend to agree @pooyan-dadvand about this. Also note that this is the general rule of the project (we use header-only solution when there is no technical alternative). Admittedly, explicit template instantiation is not nice, but it is what it is...
I tend to agree @pooyan-dadvand about this. Also note that this is the general rule of the project (we use header-only solution when there is no technical alternative). Admittedly, explicit template instantiation is not nice, but it is what it is...
There is still no cpp. Aside of this I think this is ready to go in.
I tend to agree @pooyan-dadvand about this. Also note that this is the general rule of the project (we use header-only solution when there is no technical alternative). Admittedly, explicit template instantiation is not nice, but it is what it is...
There is still no cpp. Aside of this I think this is ready to go in.
Yes sorry, the method are no longer template (I had no idea why they were in the first place) but I continue to get a strange linking error when moving the implementation to a cpp. Probably I forget some kind of definition or something I don't know about and it crashes.
I tend to agree @pooyan-dadvand about this. Also note that this is the general rule of the project (we use header-only solution when there is no technical alternative). Admittedly, explicit template instantiation is not nice, but it is what it is...
There is still no cpp. Aside of this I think this is ready to go in.
Yes sorry, the method are no longer template (I had no idea why they were in the first place) but I continue to get a strange linking error when moving the implementation to a cpp. Probably I forget some kind of definition or something I don't know about and it crashes.
From what I see in the CI it doesn't seem to be a linking error. It seems to me that you have a leftover from when you renamed the class.
I tend to agree @pooyan-dadvand about this. Also note that this is the general rule of the project (we use header-only solution when there is no technical alternative). Admittedly, explicit template instantiation is not nice, but it is what it is...
There is still no cpp. Aside of this I think this is ready to go in.
Yes sorry, the method are no longer template (I had no idea why they were in the first place) but I continue to get a strange linking error when moving the implementation to a cpp. Probably I forget some kind of definition or something I don't know about and it crashes.
From what I see in the CI it doesn't seem to be a linking error. It seems to me that you have a leftover from when you renamed the class.
Besides the rename problem, the linking error was due to a forgotten KRATOS_API. Now it works correcty in the cpp.