KratosMultiphysics / Kratos

Kratos Multiphysics (A.K.A Kratos) is a framework for building parallel multi-disciplinary simulation software. Modularity, extensibility and HPC are the main objectives. Kratos has BSD license and is written in C++ with extensive Python interface.
https://kratosmultiphysics.github.io/Kratos/
Other
1.05k stars 246 forks source link

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

Open ddiezrod opened 4 weeks ago

ddiezrod commented 4 weeks ago

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.

matekelemen commented 4 weeks ago

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 commented 4 weeks ago

can someone approve then?

rubenzorrilla commented 4 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

ddiezrod commented 3 weeks ago

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