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

[External] Update pybind11 to last version #12788

Closed loumalouomega closed 4 weeks ago

loumalouomega commented 1 month ago

📝 Description

Update pybind11 to last version (2.13).

I am trying to compile Kratos with Intel LLVM and this update is required. See https://github.com/pybind/pybind11/issues/927

🆕 Changelog

loumalouomega commented 1 month ago

Making draft, CI is failing, and still having link issues with Intel LLVM

loumalouomega commented 1 month ago
Traceback (most recent call last):
  File "/__w/Kratos/Kratos/bin/Custom/kratos/tests/test_model_part.py", line 893, in test_model_part_iterators
    for subpart in model_part1.SubModelParts:
RuntimeError: return_value_policy = move, but type Kratos::PointerHashMapSet<Kratos::ModelPart, std::hash<std::string>, Kratos::ModelPart::GetModelPartName, std::shared_ptr<Kratos::ModelPart> > is neither movable nor copyable!

Clearly this requires a major refactor

matekelemen commented 1 month ago

This would be a great opportunity to store geometries in a PointerVectorSet rather than PointerHashMapSet to make them consistent with nodes, elements, conditions and MPCs ...

As far as I know, PointerHashMapSet is used exclusively to store geometries in ModelPart, and nothing else. Its interface is oddly specific for this use case, and is non-standard. Unless there's someone who enlightens me about the merits of PointerHashMapSet, I'd just remove it in favor of PointerVectorSet.

Sigh. Please ignore this, I was thinking of GeometryContainer (that wraps a PointerHashMapSet).

RiccardoRossi commented 1 month ago

As inrecall the goal of thwt container is to allow indexing by a hash id.

The important case is that of someone not giving you an id for the geoemtries but rather one wants to identify them by the connectivity.

Having said this...i am not much into the technical.details. can someone provide more insight?

matekelemen commented 1 month ago

My previous comment is irrelevant, please ignore it.

As for the error, the problem is that ModelPart::SubModelParts() returns the sub model part container by reference (instead of a shared ptr), which is not the holder type of the container. Since pybind cannot hold the container, it tries copying it, which is thankfully not allowed.

The easiest fix would be to return the sub model part container by shared ptr. However, this is impossible right now because ModelPart holds sole ownership of this container (it stores the sub model part container by value instead of shared ptr).

https://github.com/KratosMultiphysics/Kratos/blob/505d9c05c5e482c7e39ccdc1f61eb98ca47efebd/kratos/includes/model_part.h#L1978

We don't have this issue with Nodes, Elements, etc. because Mesh stores their containers by shared ptrs.

https://github.com/KratosMultiphysics/Kratos/blob/505d9c05c5e482c7e39ccdc1f61eb98ca47efebd/kratos/includes/mesh.h#L927-L935

#

Another fix would be to return a view over the sub model parts instead of the container itself. The view could be copied without any performance-related issues, and it would be also be safer because users wouldn't be able manipulate the container directly (e.g.: add or remove sub model parts to/from the container directly instead of the ModelPart interface).

loumalouomega commented 1 month ago
ERROR: test_select_node_list (test_variable_utils.TestVariableUtils)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/Kratos/Kratos/bin/Custom/kratos/tests/test_variable_utils.py", line 599, in test_select_node_list
    node_list = KratosMultiphysics.VariableUtils().SelectNodeList(KratosMultiphysics.VISCOSITY, 0.01, model_part.Nodes)
RuntimeError: return_value_policy = move, but type Kratos::PointerVectorSet<Kratos::Node, Kratos::IndexedObject, std::less<unsigned long>, std::equal_to<unsigned long>, Kratos::intrusive_ptr<Kratos::Node>, std::vector<Kratos::intrusive_ptr<Kratos::Node>, std::allocator<Kratos::intrusive_ptr<Kratos::Node> > > > is neither movable nor copyable!

We should do a view for the Nodes/Conditions/Elements as well...

loumalouomega commented 4 weeks ago

Okay, looks like with these 2 minor changes it works now

loumalouomega commented 4 weeks ago

BTW, this new version gives support for Eigen conversion

matekelemen commented 4 weeks ago

Oh whoops, I didn't notice the automerge. Sorry.

loumalouomega commented 4 weeks ago

Oh whoops, I didn't notice the automerge. Sorry.

Ups

loumalouomega commented 4 weeks ago

👍

This View is fine for now, but (in another PR) I'd like a proper View template we can use throughout Kratos (and not only for python bindings).

By proper I mean a standard-conforming container interface, complete with type aliases and proper iterators.

Kokkos uses view for everything...