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

Expose C++ Solution to clib #158

Closed
ischoegl opened this issue Jul 29, 2022 · 6 comments · Fixed by Cantera/cantera#1448
Closed

Expose C++ Solution to clib #158

ischoegl opened this issue Jul 29, 2022 · 6 comments · Fixed by Cantera/cantera#1448
Labels
feature-request New feature request

Comments

@ischoegl
Copy link
Member

ischoegl commented Jul 29, 2022

Abstract

Since the initial introduction of the C++ Solution class, it has become central to the instantiation of ThermoPhase/Kinetics/Transport objects from YAML input. It is, however, still missing from the clib interface.

Motivation

Describe the need for the proposed change:

  • What problem is it trying to solve? ... add a missing interface to clib
  • Who is affected by the change? ... developers - this is largely 'under the hood'
  • Why is this a good solution? ... reduce number of instances required to keep track of objects

Possible Solutions

Create a template<> SolutionCabinet* SolutionCabinet in analogy to existing "cabinets" for Thermo etc.

@ischoegl ischoegl added the feature-request New feature request label Jul 29, 2022
@speth
Copy link
Member

speth commented Jul 30, 2022

I think this is a bit more complicated than described. Currently, the ownership semantics of clib are pretty simple -- clib owns all the objects that it has handles to. But working with the Solution class would require being able to provide clib handles to ThermoPhase, Kinetics, and Transport objects that are held as shared_ptrs in the Solution class.

I'd suggest deferring resolution of this to a more general rewrite of the clib interface as part of #39, where the same ownership issue will raise its head again, in reference to Species and Reaction objects as well (at a minimum).

@ischoegl
Copy link
Member Author

I agree that introducing Solution to clib will be a bit of a headache. I stumbled across this in Cantera/cantera#1345, where I had to skip changes for clib.

The reason here is that I am starting to implement a C++ version of SolutionArray (as proposed in #137), where similar roadblocks will arise. For this to work, both oneD and zeroD objects need to be aware of associated Solution objects. While the components are available, they use raw pointers, which (I think) cannot be repackaged safely.

@ischoegl
Copy link
Member Author

One alternative here would be to switch Cabinet.h over to storing shared_ptr<M> rather than M*, which would allow for the construction of Solution on-the-fly. The issue here is then that raw pointers are used extensively in the C++ core. Replacing those by smart pointers is likely a worthwhile objective, but this will open its own rather complex can of worms.

@speth
Copy link
Member

speth commented Jul 30, 2022

I don't think there's any urgency of adding a C++ SolutionArray implementation to clib though, is there? I'd likewise suggest that such an addition can wait for a new, better clib. That said, some thinking about the structure and API of this C++ SolutionArray class could be useful for understanding what new-clib needs to be able to do.

Addendum: yes, I think that storing shared_ptrs would be part of the solution, though there's more to it than just that.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 30, 2022

Agreed that adding a C++ SolutionArray to the current clib is not urgent, although it will be highly useful for any API that builds on the new clib, i.e. it will become useable as a central storage object. Fwiw, there’s a branch with some initial code here ischoegl:add-onedim-hdf-export, but I do need some infrastructure first (I.e. PR Cantera/cantera#1345).

@ischoegl
Copy link
Member Author

Following on Cantera/cantera#1447, I believe one first step would be to replace raw pointers in Cabinet by smart pointers (similar to what was said above in #158 (comment)). This will allow for a construction of Solution from parts.

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

Successfully merging a pull request may close this issue.

2 participants