Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add native SolutionArray / HDF support to C++ OneDim #1385

Merged
merged 51 commits into from
Jan 12, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Sep 10, 2022

Changes proposed in this pull request

Implement native support for HDF data in C++ OneDim using HighFive. Similar to the Python h5py implementation, a SolutionArray (also in C++) acts as an intermediate container for saving/restoring of Domain1D data.

  • Implement import of HDF data to C++ SolutionArray (using existing HDF format; since Cantera 2.5)
  • Implement import of YAML data to C++ SolutionArray (using existing YAML format; since Cantera 2.6)
  • Restore Domain1D from SolutionArray
  • Save Domain1D to SolutionArray
  • Save C++ SolutionArray to HDF (make HDF format consistent with YAML structure; implemented in C++)
  • Save C++ SolutionArray to YAML
  • Link C++ SolutionArray data to Python API (likely definitely as separate PR)

Extensions to zeroD reactor networks are anticipated.

Once Solution is supported by clib, HDF can be made available to any user-facing API.

If applicable, fill in the issue number this pull request is fixing

Partially addresses Cantera/enhancements#137 and Cantera/enhancements#158

If applicable, provide an example illustrating new features this pull request is introducing

>>> sim = ct.FreeFlame(gas)
>>> sim.restore("adiabatic_flame.h5", name="mix")

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

- Add setters/getters to SolutionArray
- Also remove unnecessary code stubs
@ischoegl
Copy link
Member Author

ischoegl commented Nov 29, 2022

@speth ... while this is not fully implemented, feel free to have a look (feedback is explicitly welcome). At this point, restoring of Sim1D from existing YAML and HDF file formats via a newly implemented C++ SolutionArray::restore works. The approach mostly follows the YAML serialization, but inserts the C++ SolutionArray so various file IO formats can be supported in one central place. HDF uses BlueBrain/HighFive - thus far, I haven't experienced any major difficulties.

Remaining work for this PR involves implementing SolutionArray::save for HDF and YAML. It's not a major effort, but it may take me a couple of days to get back to this.

@ischoegl
Copy link
Member Author

ischoegl commented Jan 5, 2023

Thanks, @ischoegl, I think these updates look good. I found a few more minor issues on a second pass. I somehow messed up using the GH interface, so there are a few comments that are in the preceding review.

@speth … thanks for the additional checks - I believe everything is addressed now.

One additional item: I think what ends up in cantera.pc needs to be double-checked. I tried this on Ubuntu using the paths from the GitHub Actions config, and I think both the library and include directories are missing.

Until this came up I was actually unaware of cantera.pc - I’m not sure how to address this as I’m not familiar with any of the details (sorry). Edit: Not too hard to figure out after all.

As an aside, on my system (macOS), the pkg-config route does not work, with cantera.pc being:

prefix=/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev
exec_prefix=${prefix}/bin
libdir=${prefix}/lib
includedir=${prefix}/include

Name: Cantera
Description: Cantera library
URL: https://cantera.org
Version: 3.0.0a3

Libs: -L${libdir} -L/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/lib -L/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/lib/python3.10/config-3.10-darwin -L/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/lib -lcantera -lsundials_cvodes -lsundials_ida -lsundials_nvecserial -lsundials_sunlinsollapackdense -lsundials_sunlinsollapackband -lhdf5 -lfmt -lyaml-cpp -lpython3.10 -ldl
Cflags: -std=c++14 -framework Accelerate -I${includedir} -I/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/include -I

An attempt to compile the very simple C++ program fails with:

% export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/lib
% export PKG_CONFIG_PATH=$PKG_CONFIG_PATH:/opt/homebrew/Caskroom/miniforge/base/envs/cantera-dev/lib/pkgconfig
% g++ simple.cpp -o myProgram $(pkg-config --cflags --libs cantera)
% ./myProgram
dyld[92219]: Library not loaded: @rpath/libsundials_cvodes.6.dylib
  Referenced from: <E62AB15F-607D-3047-996E-A59885DCC71C> /Volumes/Data/work/GitHub/cantera/myProgram
  Reason: tried: '/System/Volumes/Preboot/Cryptexes/OS@rpath/libsundials_cvodes.6.dylib' (no such file), '/usr/local/lib/libsundials_cvodes.6.dylib' (no such file), '/usr/lib/libsundials_cvodes.6.dylib' (no such file, not in dyld cache)
zsh: abort      ./myProgram

As I get the same failure for the main branch, I cannot further investigate and/or verify that my updates are correct (I switched to macOS relatively recently, and may not be aware of all differences yet - this may just not work; - it may also be an issue with compiling under a conda environment).

@ischoegl ischoegl requested review from speth January 8, 2023 14:57
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl, this all looks good to me. There is just one very minor issue with the .mak file generation, I think.

As far as the cantera.pc, I guess that requires getting the right rpath flags in when linking to shared libraries that live in non-system locations, such as a Conda install. I'd suggest we just ignore that here, and you can create a new issue to remind us that needs to be fixed at some point.

platform/posix/SConscript Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

Thanks, @ischoegl, this all looks good to me. There is just one very minor issue with the .mak file generation, I think.

Thanks for catching - fixed!

As far as the cantera.pc, I guess that requires getting the right rpath flags in when linking to shared libraries that live in non-system locations, such as a Conda install. I'd suggest we just ignore that here, and you can create a new issue to remind us that needs to be fixed at some point.

See #1422.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants