thermotools / thermopack

Thermopack is a thermodynamic model library for fluid properties and PVT calculations
Other
50 stars 13 forks source link

Added Cubic and Thermo cpp / h #169

Open kosbreiev opened 1 month ago

kosbreiev commented 1 month ago

Added the following files:

cubic.cpp cubic.h thermo.cpp thermo.h

for C++ code handling

vegardjervell commented 1 month ago

Hi,

Thanks for taking a look at the C++ wrapping here! This looks good, but it looks like there is some re-implementation of the existing C++ wrapper, with some more wrapped functionality for the cubic class (see cppExamples for usage of the existing wrapper).

If we could migrate the wrapped functionality here to that wrapper that would be nice. The existing wrapper uses some macros (see dllmacros.h) to handle different styles of name-mangling used by different fortran-compilers, and is set up to be compatible with cmake's find_library. As of now, it's written as a header-only wrapper, but whether we should split it into header/src is a fair discussion to have. Currently, my impression is that splitting it into header/src adds some complexity that isn't strictly necessary (the compile time for the existing wrapper is negligible).

Please let me know if you would like some help migrating these additions to the existing wrapper.

kosbreiev commented 1 month ago

Hi @vegardjervell !

Many thanks for your input.

The main idea was to follow the pattern we have right now with the python implementation.

For me its not really clear how thermopack library is connected with the thermo.py and cubic.py.

But in order to be consistent, I implemented cubic / thermo cpp + h(pp). First I used only headers, but as I am a frontend engineer, I have lack of knowledge in CPP, so our experienced colleague made a review and suggested to split the implementation with the declaration. I guess this is a good practice.

We can involve @ailoa and @morteham in this discussion to have this contribution valuable.

The most important is that CO2 table is generated, all the thermopack functions that is used in the app are working and we dont have any segmentation issues :)

But for sure, I'm happy to have a discussion how you guys see this functionality implemented.

CC @stigh

vegardjervell commented 1 month ago

It's great to hear that this is working out well for you guys! I have no issue with this wrapper per se, I think it's a good idea to have a C++ wrapper that is as similar as possible to the Python wrapper. My only point is really that we already have a C++ wrapper that exposes most of the core functionality in thermo, and which is in use over in KineticGas. The existing C++ wrapping system is also set up to be able to handle different name-mangling schemes used by different fortran-compilers, and is made to handle all linking to the thermopack dylib at link time, so that dlopen is unnecessary.

What I'm saying is just that, for the sake of maintainability, it would be best if we have a single C++ wrapper. The wrapper you have here exposes some functionality that isn't exposed in the existing wrapper:

If you could port the wrapping code for those methods into the existing wrapper (which it looks to me like should almost be a copy-paste) that would be great! I think your existing codebase should work with the existing wrapper with little or no modifications. That would also give the benefit of having access to the other exposed functionality in the wrapper, and the fact that we are using that wrapper in-house as well, meaning that it will be tested and maintained by more people.