Replace convenience classes by C++ Solution object

[ischoegl]

This PR introduces the C++ Solution object (previously only instantiated from the Python front-end) to the C++ test suite and examples.

Changes proposed in this pull request:

• implement initialization of C++ Solution objects from input files
• replace convenience wrapper classes (i.e. IdealGasMix, Interface) for test_problems and samples
• deprecate convenience wrappers (instead of porting them to YAML/AnyMap)

[speth]

I'm very much in favor of getting rid of these supposed "convenience" classes, and I think this is a good start to extending the use of the Solution class.

I don't understand the need for the methods getIdealGasPhasePtr and getGasKineticsPtr - I don't think there are any important methods of these classes that aren't available from a pointer to the base class. For surface phases / kinetics, there are a few unique methods like getCoverages, but I'd be inclined to handle those by having a class analogous to Solution which always returns instances of SurfPhase and InterfaceKinetics. Naming this class Interface might be slightly complicated by the existing, now-deprecated Interface class, though.

After seeing the Solution class in action a bit more, I'm wondering if it would make more sense for the thermo() method and friends to return shared_ptr and avoid the need to also having a thermoPtr() (etc.) method. If you then wanted a reference, you could always just write auto& gas = *sol.thermo();.

[ischoegl]

I don't understand the need for the methods getIdealGasPhasePtr and getGasKineticsPtr

This is true, but for some of the older examples, having to type

std::dynamic_pointer_cast<IdealGasPhase>(sol->thermoPtr())

and

std::dynamic_pointer_cast<GasKinetics>(sol->kineticsPtr())

every time is pretty onerous. As I didn't want to introduce some of the nastier looking C++ 11 features in the simple example, I decided to create the 'convenience' accessors.

The main issue is that a more straight-forward type cast of a pointer to the base class is currently blocked here

cantera/include/cantera/thermo/Phase.h

Lines 102 to 103 in ce47733

	Phase(const Phase&) = delete;
	Phase& operator=(const Phase&) = delete;

After seeing the Solution class in action a bit more, I'm wondering if it would make more sense for the thermo() method and friends to return shared_ptr and avoid the need to also having a thermoPtr() (etc.) method.

Agreed. From what I've seen, there isn't a way around having shared_ptr's. I also agree that both aren't necessary, although sometimes one or the other is more convenient. I named things such that it is immediately clear what is an object and what is a (shared) pointer.

If you want me to switch to shared_ptr only, now is the time ...

[ischoegl]

@speth ... alright. I consolidated everything to shared_ptr. Since the C++ Solution class is brand-new, it's probably better to do this now.

[speth]

This is true, but for some of the older examples, having to type

std::dynamic_pointer_cast<IdealGasPhase>(sol->thermoPtr())

[...] every time is pretty onerous.

Under what circumstances is a shared_ptr<IdealGasPhase> needed rather than a shared_ptr<ThermoPhase>? The only case I see is as the argument to the StFlow constructor, and in that case I think the preferred fix might be to change it to accept a ThermoPhase* and then cast that to IdealGasPhase* internally (and throw if that fails).

The main issue is that a more straight-forward type cast of a pointer to the base class is currently blocked here

cantera/include/cantera/thermo/Phase.h

Lines 102 to 103 in ce47733

	Phase(const Phase&) = delete;
	Phase& operator=(const Phase&) = delete;

Can you give an example of what you think is being blocked? The quoted code should not affect any pointer casting, but just disable copying and assignment.

[ischoegl]

@speth ... yes, there really isn't any special feature in IdealGasPhase where dynamic polymorphism doesn't already do the trick. I.e. I removed all of the accessors (i.e. getIdealGasPhase and similar). In some instances, a dynamic_pointer_cast is still necessary (e.g. for the freeflame sample, and the surfkin test problem; I think it's ok to let users figure those out)

The only case I see is as the argument to the StFlow constructor, and in that case I think the preferred fix might be to change it to accept a ThermoPhase* and then cast that to IdealGasPhase* internally (and throw if that fails).

I haven't dont that yet, but it's straight-forward. Edit: Done. The one case where a fix is less intuitive is the surfkin test problem, where coverages etc. aren't defined in the base class. Not sure whether it's worth creating a virtual method for setCoverages in the base class.

Regarding the blocked type cast, I misspoke. It appears to be difficult to change the type once a variable is passed by reference, but casting of pointers works regardless (I guess my original point is moot). Since much of the passing happens by reference, I was overly cautious.

[speth]

Thanks, this is looking pretty good. I mostly have just a few suggestions for simplifications and a couple of minor formatting nitpicks.

[ischoegl]

If we consolidate these methods, then there's never any question about whether to use Solution::create or newSolution, or why there's this constructor that can't be called.

That's a fair point. Will update once our PROCI manuscript is submitted (or I need to take a break) ...

[ischoegl]

Done, i.e. newSolution(infile, name, transport, adjacent) is now the default way to instantiate.

[codecov]

Codecov Report

Merging #735 into master will increase coverage by 0.01%.
The diff coverage is 94.85%.

@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
+ Coverage   70.86%   70.87%   +0.01%     
==========================================
  Files         374      372       -2     
  Lines       43683    43684       +1     
==========================================
+ Hits        30954    30963       +9     
+ Misses      12729    12721       -8

Impacted Files	Coverage Δ
include/cantera/transport/TransportBase.h	11.11% <ø> (ø)	⬆️
test_problems/surfSolverTest/surfaceSolver.cpp	76.07% <ø> (ø)	⬆️
include/cantera/kinetics/Kinetics.h	56.75% <ø> (ø)	⬆️
include/cantera/thermo/Phase.h	100% <ø> (ø)	⬆️
test_problems/surfSolverTest/surfaceSolver2.cpp	78.1% <ø> (ø)	⬆️
include/cantera/kinetics/InterfaceKinetics.h	28.57% <ø> (ø)	⬆️
include/cantera/oneD/StFlow.h	96.03% <ø> (ø)	⬆️
include/cantera/thermo/IdealGasPhase.h	96.87% <ø> (ø)	⬆️
include/cantera/thermo/SurfPhase.h	100% <ø> (ø)	⬆️
test_problems/VPsilane_test/silane_equil.cpp	66.66% <ø> (ø)	⬆️
... and 19 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d02e9...6693eff. Read the comment docs.