[Core][Geometries] Adding geometry order information#12204
[Core][Geometries] Adding geometry order information#12204loumalouomega merged 14 commits intomasterfrom
Conversation
What a beautiful world we live on in which we can experience the joy of compiling for multiple platforms and operating systems
|
@jcotela do we proceed? |
| * @var KratosGeometryOrderType::Kratos_Quintic_Order: Defines a quintic geometry. Represents the highest polynomial order here, with the highest accuracy and computational cost. | ||
| * @var KratosGeometryOrderType::Kratos_Unknown_Order: Used when the geometry order is unknown or not specified. Allows for flexibility in geometry definition. | ||
| */ | ||
| enum KratosGeometryOrderType { |
There was a problem hiding this comment.
this will be very difficult to use. I'd rather recommend an optional int
There was a problem hiding this comment.
An enum can always be converted to a int, and the order coincides with the enum value
There was a problem hiding this comment.
but why go the roundabout way of defining it as an enum in the first place? Why is an enum better here than an std::optional<int>? As far as I see it, the enum arbitrarily restricts the orders we can represent.
There was a problem hiding this comment.
We already use enums in many things for geometries, would you thinks it would be easier to just simply use unsigned int and end of story? opinions @KratosMultiphysics/technical-committee
There was a problem hiding this comment.
would you thinks it would be easier to just simply use unsigned int and end of story
an unisgned wouldn't be able to represent undefined order, and I think the undefined state of an std::optional fits better than using -1 with signed integers, or some other arbitrary stuff.
There was a problem hiding this comment.
@jcotela do you agree?, opinion @KratosMultiphysics/technical-committee ?
There was a problem hiding this comment.
Hi @loumalouomega sorry for the delay, I had forgotten about this PR. Unfortunately, this change was intended for a feature that we won't be implementing so strictly speaking I no longer need it. Having said that, I think it may be useful to have a way to programmatically inquire about the interpolation order of a specific geometry instance, if the core developers agree.
Regarding the enum vs std::optional, I understand that the order is a category here (the geometry is lineal or quadratic in the same way it is a linear triangle or a cubic hexhahedron) so an enum seems appropriate. I could see using an int so that values have a natural greater/less than comparison in addition to equality checks, but I am not sure about the need for optional. After all, it should be straightforward to figure out the interpolation order for each derived geometry. @matekelemen what use case do you see for an undefined order? Working with base geometery instances?
There was a problem hiding this comment.
I will not touch this PR until a consencus is reached
There was a problem hiding this comment.
what use case do you see for an undefined order?
Well, there's a Kratos_Unknown_Order in the current PR so I thought there's a use case for it. Also geometries that don't overload this function should return an undefined value instead of some made-up default.
I'm not familiar with IGA in kratos, but I could imagine an undefined order for a default-constructed IGA geometry as well.
There was a problem hiding this comment.
If everyone agreess I can change that
|
What is the status of this? |
I'm not blocking, just recommended an optinal int instead of an enum. I think we're still waiting for a review from the technical committee. |
|
@KratosMultiphysics/technical-committee we should retake this |
|
My comment is non-blocking, but my opinion still stands. Waiting for a review by the technical committee. |
We'll have a look during next meeting. |
|
@KratosMultiphysics/technical-committee delegates the final decision about this to @jcotela |
|
Hello @jcotela? |
jcotela
left a comment
jcotela
left a comment
There was a problem hiding this comment.
@loumalouomega sorry for the delay in answering. I'm a bit split on this.
On the one hand, I think that what you implemented here is good to go. I am not convinced by the arguments for an optional int, since I do not see a use case where the integration order is undefined at run-time. I can see how it may be unknown at compile time, for example for nurbs-type geometries, but a given geometry instance should be able to determine its order. If the order can not be undefined at run-time, I would not burden the user of the method with checking for a valid optional.
On the other hand, I originally intended to use this to distinguish between linear and quadratic elements but we ended up not using it. I now believe that this was not the right choice for my problem. In particular, I'm thinking about Taylor-Hood P2P1 type elements, where being "linear" or "quadratic" depends on the DOF. In that case, only the element (actually the GeometricalObject in kratos terms) knows the right answer, and even then it can change for a given DOF.
So in summary, I think the code is good, but I am not sure we are addressing a real problem with this. If this was my work, I would close the PR to avoid adding dead code. I will approve in the hope that this is useful for someone else.
6 participants
📝 Description
This PR defines a method in order to retrieve the geometry order of a geometry. This is a request by @jcotela to simplify its current workflow.
🆕 Changelog
KratosGeometryOrderTypeKratosGeometryOrderTypeKratos_Zero_OrdergeometriesKratos_Linear_OrdergeometriesKratos_Quadratic_OrdergeometriesKratos_Cubic_OrdergeometriesKratos_Quartic_Ordergeometries