open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
54 stars 10 forks source link

first version of waterfallFixedStep algorithm: ordering simulator tas… #734

Open xmirabel opened 1 year ago

xmirabel commented 1 year ago

…ks ok, but output variable dispatch bug: mutex issue?

taskflow is a thirdparty library: is it the right way to include it?

kyllingstad commented 11 months ago

Thanks for your efforts and your willingness to contribute your work to libcosim!

Some changes are needed before we can even start to review this:

In most cases, the addition of a new co-simulation algorithm should be limited to adding one header and one .cpp file in cosim/algorithm. (Though in some cases additional files with support code may be justified.)

Please also be aware that the fact that this algorithm depends on a third-party library will reduce the chance that we accept the contribution. Extra dependencies come with a cost even if they are pulled in via Conan. They increase the complexity of the build system, increase build times, make it harder to support multiple platforms, and occasionally break things because upstream developers do things they shouldn't have done. I am not familiar with Taskflow, so I don't know how mature or robust it is, but these are factors which we'll need to consider.

I am not saying these things to discourage you, only to be honest about the fact that we have limited resources for review and maintenance and therefore need to be selective about which contributions we accept.

xmirabel commented 11 months ago

Hello Lars, This contribution is a first test to understand the process of merging with you in an open source classic way. This is the first time I do this and I do not know the classic usage.

First, the condition of work we would like to use are:

This implies:

I agree that these changes could be done appart as a specific branch, because they solve compatibilities issues of a new usage.

xmirabel commented 11 months ago

The main goal of this branch is described in libcosimc issues:

I did not know if they should be duplicated/moved here or somewhere else?

The taskflow library is a template library (only include files) found at https://taskflow.github.io/taskflow/index.html

I do not know how to include it in conan and vcpkg and cmake. I will need help on it.

The usage of this library is fundamental for what we want to achieve. It is a very light one but with a lot of performances.

kyllingstad commented 10 months ago

The main goal of this branch is described in libcosimc issues: [...] I did not know if they should be duplicated/moved here or somewhere else?

Ah, ok, I didn't think to look for the issues there. I have transferred them to libcosim now. (libcosimc is just a thin wrapper library.)

I do not know how to include it in conan and vcpkg and cmake. I will need help on it.

Taskflow already exists as a Conan package in the Conan-center repository: https://conan.io/center/recipes/taskflow?version=3.6.0 So "all" you need to do is to include it along with the other packages in the Conanfile and update the CMakeFiles accordingly. (I put "all" in scare quotes because it is usually not that simple.) You may want to check out the Conan documentation to learn more about how to do this. Warning: Conan has been released in a new major version, v. 2, but we are still on version 1. They are not entirely compatible. We are in the process of upgrading to Conan 2 ourselves, but it is taking some time.