[Thermo] addressing review comments for C++ Solution object

include/cantera/base/Solution.h

@@ -18,9 +18,11 @@ class Transport;
 //! A container class holding managers for all pieces defining a phase
 class Solution : public std::enable_shared_from_this<Solution>
 {
-public:
+
+private:
     Solution();
-    Solution(const std::string& infile, const std::string& phasename);
+
+public:
     ~Solution() {}
     Solution(const Solution&) = delete;
     Solution& operator=(const Solution&) = delete;
@@ -35,9 +37,6 @@ class Solution : public std::enable_shared_from_this<Solution>
     //! Set the name of this Solution object
     void setName(const std::string& name);
 
-    //! Generate self-documenting YAML string
-    std::string toYAML() const;
-
     //! Set the ThermoPhase object
     void setThermoPhase(shared_ptr<ThermoPhase> thermo);
 

interfaces/cython/cantera/base.pyx

@@ -73,7 +73,7 @@ cdef class _SolutionBase:
             assert self.transport is not NULL
 
         phase_name = pystr(self.thermo.id())
-        name = kwargs.get('name', None)
+        name = kwargs.get('name')
         if name is not None:
             self.name = name
         elif phase_name in _phase_counts:
@@ -86,8 +86,9 @@ cdef class _SolutionBase:
 
     property name:
         """
-        The name assigned to this SolutionBase object. The default is
-        taken from the CTI/XML/YAML input file.
+        The name assigned to this _SolutionBase object. The default is
+        taken from the CTI/XML/YAML input file (which may be appended
+        during instantiation to create a globally unique name).
         """
         def __get__(self):
             return pystr(self.base.name())
@@ -98,7 +99,7 @@ cdef class _SolutionBase:
     property composite:
         """
         Returns tuple of thermo/kinetics/transport models associated with
-        this SolutionBase object.
+        this _SolutionBase object.
         """
         def __get__(self):
             thermo = None if self.thermo == NULL \
@@ -133,13 +134,13 @@ cdef class _SolutionBase:
 
         # Kinetics
         cdef vector[CxxThermoPhase*] v
-        cdef _SolutionBase adj
+        cdef _SolutionBase phase
 
         if isinstance(self, Kinetics):
             v.push_back(self.thermo)
-            for adj in adjacent:
+            for phase in adjacent:
                 # adjacent bulk phases for a surface phase
-                v.push_back(adj.thermo)
+                v.push_back(phase.thermo)
             self._kinetics = newKinetics(v, phaseNode, root)
             self.kinetics = self._kinetics.get()
         else:

interfaces/cython/cantera/composite.py

@@ -25,9 +25,10 @@ class Solution(ThermoPhase, Kinetics, Transport):
 
         gas = ct.Solution('gri30.yaml')
 
-    If an input file defines multiple phases, the *phases* entry (in YAML),
-    *name* (in CTI), or *id* (in XML) can be used to specify the desired
-    phase via the ``phase_id`` keyword argument of the constructor::
+    If an input file defines multiple phases, the corresponding key in the 
+    *phases* map (in YAML), *name* (in CTI), or *id* (in XML) can be used
+    to specify the desired phase via the ``phase_id`` keyword argument of 
+    the constructor::
 
         gas = ct.Solution('diamond.yaml', phase_id='gas')
         diamond = ct.Solution('diamond.yaml', phase_id='diamond')

interfaces/cython/cantera/test/test_thermo.py

@@ -15,18 +15,18 @@ def setUp(self):
         self.phase = ct.Solution('h2o2.xml')
 
     def test_base_attributes(self):
-        self.assertTrue(isinstance(self.phase.name, str))
-        self.assertTrue(isinstance(self.phase.thermo_model, str))
-        self.assertTrue(isinstance(self.phase.kinetics_model, str))
-        self.assertTrue(isinstance(self.phase.transport_model, str))
-        self.assertTrue(isinstance(self.phase.composite, tuple))
-        self.assertTrue(len(self.phase.composite)==3)
-        self.assertTrue(
-            self.phase.composite == (self.phase.thermo_model,
-                                     self.phase.kinetics_model,
-                                     self.phase.transport_model))
+        self.assertIsInstance(self.phase.name, str)
+        self.assertIsInstance(self.phase.thermo_model, str)
+        self.assertIsInstance(self.phase.kinetics_model, str)
+        self.assertIsInstance(self.phase.transport_model, str)
+        self.assertIsInstance(self.phase.composite, tuple)
+        self.assertEqual(len(self.phase.composite), 3)
+        self.assertEqual(self.phase.composite,
+                         (self.phase.thermo_model,
+                          self.phase.kinetics_model,
+                          self.phase.transport_model))
         self.phase.name = 'spam'
-        self.assertTrue(self.phase.name=='spam')
+        self.assertEqual(self.phase.name, 'spam')
         with self.assertRaises(AttributeError):
             self.phase.type = 'eggs'
 
@@ -327,25 +327,25 @@ def test_phase(self):
 
         with warnings.catch_warnings(record=True) as w:
             self.assertEqual(self.phase.ID, 'ohmech')
-            self.assertTrue(len(w) == 1)
+            self.assertEqual(len(w), 1)
             self.assertTrue(issubclass(w[-1].category, DeprecationWarning))
-            self.assertTrue("To be removed after Cantera 2.5. "
-                            in str(w[-1].message))
+            self.assertIn("To be removed after Cantera 2.5. ",
+                          str(w[-1].message))
 
         with warnings.catch_warnings(record=True) as w:
             self.phase.ID = 'something'
             self.assertEqual(self.phase.phase_id, 'something')
-            self.assertTrue(len(w) == 1)
+            self.assertEqual(len(w), 1)
             self.assertTrue(issubclass(w[-1].category, DeprecationWarning))
-            self.assertTrue("To be removed after Cantera 2.5. "
-                            in str(w[-1].message))
+            self.assertIn("To be removed after Cantera 2.5. ",
+                          str(w[-1].message))
 
         with warnings.catch_warnings(record=True) as w:
             gas = ct.Solution('h2o2.cti', phaseid='ohmech')
-            self.assertTrue(len(w) == 1)
+            self.assertEqual(len(w), 1)
             self.assertTrue(issubclass(w[-1].category, FutureWarning))
-            self.assertTrue("Keyword 'phase_id' replaces 'phaseid'"
-                            in str(w[-1].message))
+            self.assertIn("Keyword 'phase_id' replaces 'phaseid'",
+                          str(w[-1].message))
 
     def test_badLength(self):
         X = np.zeros(5)
@@ -1169,7 +1169,7 @@ def test_invalid(self):
     def test_wrap(self):
         st = self.gas.species('H2O').thermo
 
-        self.assertTrue(isinstance(st, ct.NasaPoly2))
+        self.assertIsInstance(st, ct.NasaPoly2)
 
         for T in [300, 500, 900, 1200, 2000]:
             self.gas.TP = T, 101325

src/base/Solution.cpp

@@ -12,20 +12,9 @@ namespace Cantera
 {
 
 Solution::Solution() :
-    m_thermo(nullptr),
-    m_kinetics(nullptr),
-    m_transport(nullptr),
     m_name("<Solution_name>")
 {}
 
-Solution::Solution(const std::string& infile,
-                   const std::string& phasename) :
-    Solution()
-{
-    // this *may* be a spot to load all pieces of a phase
-    throw NotImplementedError("Solution constructor from file");
-}
-
 std::string Solution::name() const {
     return m_name;
 }
@@ -34,10 +23,6 @@ void Solution::setName(const std::string& name){
     m_name = name;
 }
 
-std::string Solution::toYAML() const {
-    throw NotImplementedError("Solution::toYAML");
-}
-
 void Solution::setThermoPhase(shared_ptr<ThermoPhase> thermo) {
     m_thermo = thermo;
     if (m_thermo) {