libcosim icon indicating copy to clipboard operation
libcosim copied to clipboard

first version of waterfallFixedStep algorithm: ordering simulator tas…

Open xmirabel opened this issue 2 years ago • 4 comments

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

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

xmirabel avatar Apr 20 '23 15:04 xmirabel

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:

  • The pull request description should be updated to provide a description of the change and why it's needed. I know you've talked directly to some of my collaborators about it, but since this is an open project, we also need to have the information here on GitHub for the sake of openness and traceability.
  • Third-party libraries should generally not be included in the pull request, but instead pulled in via Conan.
  • There are several changes to the libcosim code that seem unrelated to the addition of a new co-simulation algorithm. Examples include:
    • The modification of .gitignore
    • The added message() commands in CMakeLists.txt
    • The new CMakePresets.json file
    • Lots of minor changes to unrelated parts of the code, like the replacement of int with size_t in places, etc. Please only include changes that directly relate to the main purpose of the pull request.

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.

kyllingstad avatar Sep 26 '23 13:09 kyllingstad

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:

  • Developpement PC on Windows + WSL on Linux Ubuntu or (alternatively) a actual Linux server;
  • Git sandbox on Windows side;
  • Microsoft Visual Studio Pro 2022 on Windows using CMake projects with remote Linux compilation/debugging execution;
  • The last (or the nearest as possible) version of gcc compiler;
  • The last version of cmake;
  • The usage of vcpkg instead of conan (more adapted to Visual Studio).

This implies:

  • a configuration of Visual Studio using new CMakePresets.json files;
  • some bug fixes due to gcc compiler upgrade (int to size_t come from that)
  • some changes in some CMakeLists.txt (perhaps not this one is useful but some like proxyfmu are needed)

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

xmirabel avatar Sep 29 '23 08:09 xmirabel

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

  • https://github.com/open-simulation-platform/libcosim/issues/743
  • https://github.com/open-simulation-platform/libcosim/issues/744

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.

xmirabel avatar Sep 29 '23 08:09 xmirabel

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.

kyllingstad avatar Oct 09 '23 07:10 kyllingstad

Closing this due to lack of activity.

kyllingstad avatar Oct 02 '24 08:10 kyllingstad