Replace convenience classes by C++ Solution object by ischoegl · Pull Request #735 · Cantera/cantera

ischoegl · 2019-10-27T04:39:05Z

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)

interfaces/cython/cantera/examples/onedim/diffusion_flame.py

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();.

include/cantera/base/Solution.h

ischoegl · 2019-11-02T20:07:44Z

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 ...

- implement constructor that loads ThermoPhase, Kinetics, and Transport from input files (wrapping factory class methods) - logic for selection of transport manager follows Python object - add convenience methods to type-cast frequently used classes

- Remove dependence on IdealGasMix.h

- Edge.h, IncompressibleSolid.h and Metal.h

- IdealGasMix.h and Interface.h

ischoegl · 2019-11-02T21:52:40Z

@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 · 2019-11-05T05:05:23Z

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 · 2019-11-05T14:53:19Z

@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.

- allow for 'ThermoPhase`, and cast to 'IdealGasPhase' internally

speth

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

samples/cxx/demo.cpp

include/cantera/base/Solution.h

include/cantera/zeroD/Reactor.h

samples/cxx/openmp_ignition/openmp_ignition.cpp

src/base/Solution.cpp

src/oneD/StFlow.cpp

test_problems/mixGasTransport/mixGasTransport.cpp

test_problems/multiGasTransport/multiGasTransport.cpp

include/cantera/base/Solution.h

ischoegl · 2019-11-07T05:39:56Z

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 · 2019-11-08T16:18:23Z

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

codecov · 2019-11-08T16:44:55Z

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.

ischoegl force-pushed the deprecate-convenience-classes branch 5 times, most recently from ec82d24 to 9d9f588 Compare October 27, 2019 14:30

ischoegl changed the title ~~Replace convenience classes by C++ Solution~~ Replace convenience classes by C++ Solution object Oct 27, 2019

ischoegl force-pushed the deprecate-convenience-classes branch 5 times, most recently from c519b86 to 5b3c41c Compare October 28, 2019 23:50

ischoegl mentioned this pull request Oct 29, 2019

Deprecate all instances of gri30_mix and gri30_multi #736

Closed

bryanwweber reviewed Oct 30, 2019

View reviewed changes

interfaces/cython/cantera/examples/onedim/diffusion_flame.py Outdated Show resolved Hide resolved

ischoegl force-pushed the deprecate-convenience-classes branch from db19dad to 5b3c41c Compare November 1, 2019 17:13

speth reviewed Nov 2, 2019

View reviewed changes

include/cantera/base/Solution.h Outdated Show resolved Hide resolved

Ingmar Schoegl added 6 commits November 2, 2019 15:09

[test_problems] replace convenience wrapper classes by C++ Solution

14a1af9

- Remove dependence on IdealGasMix.h

[samples] replace convenience wrapper classes by C++ Solution

b2dc4c8

Deprecate (previously) unused convenience wrapper classes

39dbb31

- Edge.h, IncompressibleSolid.h and Metal.h

Deprecate (previously) used convenience wrapper classes

c95c6ec

- IdealGasMix.h and Interface.h

Replace doublereal by double

bcde29d

ischoegl force-pushed the deprecate-convenience-classes branch from 5b3c41c to bcde29d Compare November 2, 2019 20:09

ischoegl requested a review from speth November 2, 2019 22:19

[Base] consolidate shared_ptr access in Solution

cfb54d4

ischoegl force-pushed the deprecate-convenience-classes branch from 8043e5b to cfb54d4 Compare November 5, 2019 14:34

[oneD] change thermo type passed to StFlow constructor

07e0e2f

- allow for 'ThermoPhase`, and cast to 'IdealGasPhase' internally

ischoegl force-pushed the deprecate-convenience-classes branch from e4d2942 to 07e0e2f Compare November 5, 2019 22:23

speth requested changes Nov 5, 2019

View reviewed changes

speth requested changes Nov 7, 2019

View reviewed changes

include/cantera/base/Solution.h Outdated Show resolved Hide resolved

[Base] finalize introduction of Solution object in C++ layer

6693eff

ischoegl force-pushed the deprecate-convenience-classes branch from 8f8f297 to 6693eff Compare November 8, 2019 16:16

ischoegl requested a review from speth November 8, 2019 16:58

speth approved these changes Nov 8, 2019

View reviewed changes

speth merged commit 91c7bf1 into Cantera:master Nov 8, 2019

ischoegl deleted the deprecate-convenience-classes branch December 16, 2019 15:55

Uh oh!

Conversation

ischoegl commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ischoegl commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Nov 2, 2019

Uh oh!

speth commented Nov 5, 2019

Uh oh!

ischoegl commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Nov 8, 2019

Uh oh!

codecov bot commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ischoegl commented Oct 27, 2019 •

edited

Loading

ischoegl commented Nov 2, 2019 •

edited

Loading

ischoegl commented Nov 5, 2019 •

edited

Loading

ischoegl commented Nov 7, 2019 •

edited

Loading

codecov bot commented Nov 8, 2019 •

edited

Loading