Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Add variable type - vector of data value container #12812

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ddiezrod
Copy link
Contributor

We have added a Variable in altair side that stores a vector of data value containers, with the aim of using it to store values in different layers for a unique geometry. I need to modify these files in core to be able to use this new type (or at least I dont know how to do it from our own applications). I hope this is ok.

@ddiezrod ddiezrod self-assigned this Oct 31, 2024
@ddiezrod ddiezrod requested a review from a team as a code owner October 31, 2024 10:38
Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

Okay from my side

@matekelemen
Copy link
Contributor

matekelemen commented Oct 31, 2024

A Variable storing a DataValueContainer sounds scary, let alone a vector of them. That said, I'm not opposed to extending the python interface to support it.

I can't think of a way of restricting the required interface to your application though, sorry. The problem is with the member function overloads you added to Mesh (the setters and getters for the new Variable type). py::module_ doesn't store the py::class_es you defined on them (so you can't extend the bindings of a class that's already bound), and you don't have access to Core's py::module_ anyway.

@ddiezrod
Copy link
Contributor Author

can someone approve then?

loumalouomega
loumalouomega previously approved these changes Oct 31, 2024
@rubenzorrilla
Copy link
Member

Though I agree @matekelemen that this sounds scary, if you need you need it. Nevertheless, can you please describe the use case for it you have in mind? (you did it in the PR description but it is not clear to me).

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 1, 2024

@rubenzorrilla Sure! In fact, although I might need to merge this to have a solution now, I believe this can be considered a generic problem within Kratos. I need to read and store the info from an output file of a stamping simulation. They use shell elements (triangles) but then they store information in several layers within a virtual thickness (stresses, strains, effective plastic strain, ...) , so even though the data is stored at the element level, each layer contains unique values.

This is part of a project aimed at creating a generic reader, so anything I implement needs to be very flexible. I couldn’t find any other solution besides this one; I hope that makes it a bit clearer.

@matekelemen
Copy link
Contributor

Well, I think the questionable decision there is choosing the geometry to be 2-dimensional, but then artificially extending it.

An Accessor would be able to help you if the geometry was 3D and had a thickness.

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 1, 2024

@matekelemen, Im afraid I have no control at all over the input from the stamping simulation, as this is provided by a different solver. However, I believe that using 2D geometries to simulate this kind of process is quite standard...

@matekelemen
Copy link
Contributor

Got it. If it really is standard, then we may have to start thinking about an abstraction for virtual dimensions (obviously not within the scope of this PR though).

@sunethwarna
Copy link
Member

sunethwarna commented Nov 1, 2024

I also agree with @matekelemen . This may open up a whole lot of possibilities for misuse. If it is necessary, then I am also not sure yet how to make it safe.

Maybe instead of using a DataValueContainer, would it be possible to make a custom class with a clear name and define a variable for it so at least then it has a purpose (still can be misused, but at least the name of the class won't make sense misuse cases)?

Eg. ConstitutiveLaw, AdjointExtension variables.

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 1, 2024

@sunethwarna Im not sure if I understand your suggestion. I could create a specific class for it, but in any case this new class should still wrap up a DataValueContainer to store the final data, which could also be misused in my opinion...

@sunethwarna
Copy link
Member

That is exactly what I am saying. Until now no one we know used CONSTITUTIVE_LAW variable in any other place except for the Properties. Same for the ADJOINT_EXTENSION variable. So if you give a specific meaning to the class with what you want to achieve in your case, then people will be reluctant to misuse it.

I don't see a way to stop misuse this unless we add another template Parameter to variables indicating in which containers these variables can be used and implementing interfaces for those variables in specific containers. (That needs further discussion for sure. Just a rough idea just came to my mind )

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 1, 2024

@sunethwarna Ok, I think I get your point and I think its a good idea. It might also help us to create a more clear interface if needed. If no one has anything against this idea, I will go for it.

@rubenzorrilla
Copy link
Member

Just saying that I like @sunethwarna's suggestion. Nevertheless, I'm adding this to the next @KratosMultiphysics/technical-committee discussion to give you final confirmation.

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 2, 2024

@rubenzorrilla thanks! As we are already thinking about this, I think it is important to take into account that the next step would be to be able to store values at gauss points. So for example, you have a triangle with 5 layers and each layer has 3 gauss points, so we will need to be able to store information for the 15 points.

@RiccardoRossi
Copy link
Member

@KratosMultiphysics/technical-committee delegates the final decision on this to @pooyan-dadvand.

@ddiezrod
Copy link
Contributor Author

ddiezrod commented Nov 4, 2024

I added a new class as agreed, please review when you can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

6 participants