Fixes Indexed assignment of extra columns in SolutionArray

.github/workflows/main.yml

@@ -151,10 +151,10 @@ jobs:
           --exclude='/doxygen/xml' --delete --delete-excluded \
           "${DOCS_OUTPUT_DIR}" ${RSYNC_USER}@${RSYNC_SERVER}:${RSYNC_DEST}
 
-  ubuntu-1604-py2:
-    # Ubuntu 16.04 using Python 2 to run SCons and the system Python 3 for the interface
-    name: Python 2 running SCons on Ubuntu 16.04
-    runs-on: ubuntu-16.04
+  ubuntu-1804-py2:
+    # Ubuntu 18.04 using Python 2 to run SCons and the system Python 3 for the interface
+    name: Python 2 running SCons on Ubuntu 18.04
+    runs-on: ubuntu-18.04
     steps:
       - uses: actions/checkout@v2
         name: Checkout the repository
@@ -163,12 +163,9 @@ jobs:
       - name: Install Apt dependencies
         run: |
           sudo apt-get install libboost-dev gfortran scons python3-numpy \
-          python3-pip python3-setuptools libsundials-serial-dev liblapack-dev \
+          python3-pip python3-setuptools python3-h5py python3-pandas \
+          python3-ruamel.yaml cython libsundials-dev liblapack-dev \
           libblas-dev
-      - name: Install Python dependencies
-        # Don't include Pandas here due to install errors
-        run: |
-          sudo -H /usr/bin/python3 -m pip install ruamel.yaml cython h5py
       - name: Build Cantera
         run: scons build python_cmd=/usr/bin/python3 blas_lapack_libs=lapack,blas -j2
       - name: Test Cantera

AUTHORS

@@ -44,6 +44,7 @@ Jeff Santner (@jsantner)
 Satyam Saxena (@CyberDrudge)
 Ingmar Schoegl (@ischoegl), Louisiana State University
 Santosh Shanbhogue (@santoshshanbhogue), Massachusetts Institute of Technology
+Harsh Sinha (@sin-ha)
 David Sondak
 Raymond Speth (@speth), Massachusetts Institute of Technology
 Sergey Torokhov (@band-a-prend)

interfaces/cython/cantera/composite.py

@@ -529,32 +529,50 @@ def __init__(self, phase, shape=(0,), states=None, extra=None, meta=None):
         reserved = self.__dir__()
 
         self._extra = OrderedDict()
+
         if isinstance(extra, dict):
             for name, v in extra.items():
                 if name in reserved:
                     raise ValueError(
                         "Unable to create extra column '{}': name is already "
                         "used by SolutionArray objects.".format(name))
                 if not np.shape(v):
-                    self._extra[name] = [v]*self._shape[0]
-                elif len(v) == self._shape[0]:
-                    self._extra[name] = list(v)
+                    self._extra[name] = np.full(self._shape, v)
+                elif np.array(v).shape == self._shape:
+                    self._extra[name] = np.array(v)
                 else:
-                    raise ValueError("Unable to map extra SolutionArray"
-                                     "input for named {!r}".format(name))
+                    raise ValueError("Unable to map extra SolutionArray "
+                                     "input named {!r}".format(name))
+        elif extra is not None:
+            if self._shape != (0,):
+                raise ValueError("Initial values for extra properties must be "
+                                 "supplied in a dictionary if the SolutionArray "
+                                 "is not initially empty.")
+            if isinstance(extra, np.ndarray):
+                extra = extra.flatten()
+            elif isinstance(extra, str):
+                extra = [extra]
 
-        elif extra and self._shape == (0,):
-            for name in extra:
+            try:
+                iter_extra = iter(extra)
+            except TypeError:
+                raise ValueError(
+                    "Extra properties can be created by passing an iterable "
+                    "of names for the properties. If you want to supply initial "
+                    "values for the properties, use a dictionary whose keys are "
+                    "the names of the properties and values are the initial "
+                    "values.") from None
+
+            for name in iter_extra:
+                if not isinstance(name, str):
+                    raise TypeError(
+                        "Unable to create extra column, passed value '{!r}' "
+                        "is not a string".format(name))
                 if name in reserved:
                     raise ValueError(
                         "Unable to create extra column '{}': name is already "
                         "used by SolutionArray objects.".format(name))
-                self._extra[name] = []
-
-        elif extra:
-            raise ValueError("Initial values for extra properties must be "
-                             "supplied in a dict if the SolutionArray is not "
-                             "initially empty")
+                self._extra[name] = np.empty(shape=(0,))
 
         if meta is None:
             self._meta = {}
@@ -574,7 +592,7 @@ def __getitem__(self, index):
 
     def __getattr__(self, name):
         if name in self._extra:
-            return np.array(self._extra[name])
+            return self._extra[name]
         else:
             raise AttributeError("'{}' object has no attribute '{}'".format(
                 self.__class__.__name__, name))
@@ -608,28 +626,54 @@ def append(self, state=None, **kwargs):
         if len(self._shape) != 1:
             raise IndexError("Can only append to 1D SolutionArray")
 
-        for name, value in self._extra.items():
-            value.append(kwargs.pop(name))
+        # This check must go before we start appending to any arrays so that
+        # array lengths don't get out of sync.
+        missing_extra_kwargs = self._extra.keys() - kwargs.keys()
+        if missing_extra_kwargs:
+            raise TypeError(
+                "Missing keyword arguments for extra values: "
+                "'{}'".format(", ".join(missing_extra_kwargs))
+            )
+
+        # For the checks of the state below, the kwargs dictionary can
+        # only contain keywords that match properties of the state. Here
+        # we pop any kwargs that have to do with the extra items so they
+        # aren't included in that check. They are put into a temporary
+        # storage so that appending can be done at the end of the function
+        # all at once.
+        extra_temp = {}
+        for name in self._extra:
+            extra_temp[name] = kwargs.pop(name)
 
         if state is not None:
             self._phase.state = state
 
         elif len(kwargs) == 1:
             attr, value = next(iter(kwargs.items()))
             if frozenset(attr) not in self._phase._full_states:
-                raise KeyError("{} does not specify a full thermodynamic state")
+                raise KeyError(
+                    "'{}' does not specify a full thermodynamic state".format(attr)
+                )
             setattr(self._phase, attr, value)
 
         else:
             try:
                 attr = self._phase._full_states[frozenset(kwargs)]
             except KeyError:
-                raise KeyError("{} is not a valid combination of properties "
-                    "for setting the thermodynamic state".format(tuple(kwargs)))
+                raise KeyError(
+                    "{} is not a valid combination of properties for setting "
+                    "the thermodynamic state".format(tuple(kwargs))
+                ) from None
             setattr(self._phase, attr, [kwargs[a] for a in attr])
 
         self._states.append(self._phase.state)
         self._indices.append(len(self._indices))
+        for name, value in self._extra.items():
+            # Casting to a list before appending is ~5x faster than using
+            # np.append when appending a single item.
+            v = value.tolist()
+            v.append(extra_temp.pop(name))
+            self._extra[name] = np.array(v)
         self._shape = (len(self._indices),)
 
     @property
@@ -655,7 +699,7 @@ def sort(self, col, reverse=False):
             indices = indices[::-1]
         self._states = [self._states[ix] for ix in indices]
         for k, v in self._extra.items():
-            self._extra[k] = list(np.array(v)[indices])
+            self._extra[k] = v[indices]
 
     def equilibrate(self, *args, **kwargs):
         """ See `ThermoPhase.equilibrate` """
@@ -865,7 +909,6 @@ def collect_data(self, cols=None, tabular=False, threshold=0, species=None):
         if tabular and len(self._shape) != 1:
             raise AttributeError("Tabular output of collect_data only works "
                                  "for 1D SolutionArray")
-        out = OrderedDict()
 
         # Create default columns (including complete state information)
         if cols is None:
@@ -889,7 +932,6 @@ def collect_data(self, cols=None, tabular=False, threshold=0, species=None):
 
         def split(c, d):
             """ Split attribute arrays into columns for tabular output """
-            single_species = False
             # Determine labels for the items in the current group of columns
             if c in self._n_species:
                 collabels = ['{}_{}'.format(c, s) for s in self.species_names]

interfaces/cython/cantera/test/test_thermo.py

@@ -1725,15 +1725,83 @@ def test_slicing_ndim(self):
         self.assertArrayNear(col3.T, 900*np.ones(2))
         self.assertArrayNear(row2.T, 900*np.ones(5))
 
-    def test_extra(self):
+    def test_extra_create_by_dict(self):
         extra = OrderedDict([('grid', np.arange(10)),
                              ('velocity', np.random.rand(10))])
         states = ct.SolutionArray(self.gas, 10, extra=extra)
         keys = list(states._extra.keys())
         self.assertEqual(keys[0], 'grid')
-
-        with self.assertRaises(ValueError):
-            states = ct.SolutionArray(self.gas, extra=['creation_rates'])
+        self.assertArrayNear(states.grid, np.arange(10))
+
+    def test_extra_no_shape(self):
+        # The shape of the value for "prop" here is (), which is falsey
+        # and causes the use of np.full()
+        states = ct.SolutionArray(self.gas, 3, extra={"prop": 1})
+        self.assertEqual(states.prop.shape, (3,))
+        self.assertArrayNear(states.prop, np.array((1, 1, 1)))
+
+        # Check a multidimensional SolutionArray
+        states = ct.SolutionArray(self.gas, (2, 2), extra={"prop": 3})
+        self.assertEqual(states.prop.shape, (2, 2))
+        self.assertArrayNear(states.prop, np.array(((3, 3), (3, 3))))
+
+    def test_extra_not_empty(self):
+        """Test that a non-empty SolutionArray raises a ValueError if
+           initial values for properties are not supplied.
+        """
+        with self.assertRaisesRegex(ValueError, "Initial values for extra properties"):
+            ct.SolutionArray(self.gas, 3, extra=["prop"])
+        with self.assertRaisesRegex(ValueError, "Initial values for extra properties"):
+            ct.SolutionArray(self.gas, 3, extra=np.array(["prop", "prop2"]))
+
+    def test_extra_wrong_shape(self):
+        with self.assertRaisesRegex(ValueError, "Unable to map"):
+            ct.SolutionArray(self.gas, (3, 3), extra={"prop": np.arange(3)})
+
+    def test_extra_create_by_iterable(self):
+        states = ct.SolutionArray(self.gas, extra=("prop1"))
+        self.assertEqual(states.prop1.shape, (0,))
+
+        # An integer is not an iterable, and only bare strings are
+        # turned into iterables
+        with self.assertRaisesRegex(ValueError, "Extra properties"):
+            ct.SolutionArray(self.gas, extra=2)
+
+    def test_extra_not_string(self):
+        with self.assertRaisesRegex(TypeError, "is not a string"):
+            ct.SolutionArray(self.gas, extra=[1])
+
+    def test_extra_reserved_names(self):
+        with self.assertRaisesRegex(ValueError, "name is already used"):
+            ct.SolutionArray(self.gas, extra=["creation_rates"])
+
+        with self.assertRaisesRegex(ValueError, "name is already used"):
+            ct.SolutionArray(self.gas, extra={"creation_rates": 0})
+
+    def test_extra_create_by_string(self):
+        states = ct.SolutionArray(self.gas, extra="prop")
+        self.assertEqual(states.prop.shape, (0,))
+
+    def test_assign_to_slice(self):
+        states = ct.SolutionArray(self.gas, 7, extra={'prop': range(7)})
+        array = np.arange(7)
+        self.assertArrayNear(states.prop, array)
+        states.prop[1] = -5
+        states.prop[3:5] = [0, 1]
+        array_mod = np.array([0, -5, 2, 0, 1, 5, 6])
+        self.assertArrayNear(states.prop, array_mod)
+
+    def test_extra_create_by_ndarray(self):
+        properties_array = np.array(["prop1", "prop2", "prop3"])
+        states = ct.SolutionArray(self.gas, shape=(0,), extra=properties_array)
+        self.assertEqual(states.prop1.shape, (0,))
+        self.assertEqual(states.prop2.shape, (0,))
+        self.assertEqual(states.prop3.shape, (0,))
+        # Ensure that a 2-dimensional array is flattened
+        properties_array = np.array((["prop1"], ["prop2"]))
+        states = ct.SolutionArray(self.gas, extra=properties_array)
+        self.assertEqual(states.prop1.shape, (0,))
+        self.assertEqual(states.prop2.shape, (0,))
 
     def test_append(self):
         states = ct.SolutionArray(self.gas, 5)
@@ -1753,12 +1821,46 @@ def test_append(self):
 
         self.gas.TPX = 300, 1e4, 'O2:0.5, AR:0.5'
         HPY = self.gas.HPY
-        self.gas.TPX = 1200, 5e5, 'O2:0.3, AR:0.7' # to make sure it gets changed
+        self.gas.TPX = 1200, 5e5, 'O2:0.3, AR:0.7'  # to make sure it gets changed
         states.append(HPY=HPY)
         self.assertEqual(states.cp_mass.shape, (8,))
         self.assertNear(states.P[-1], 1e4)
         self.assertNear(states.T[-1], 300)
 
+    def test_append_with_extra(self):
+        states = ct.SolutionArray(self.gas, 5, extra={"prop": "value"})
+        states.TPX = np.linspace(500, 1000, 5), 2e5, 'H2:0.5, O2:0.4'
+        self.assertEqual(states._shape, (5,))
+        states.append(T=1100, P=3e5, X="AR:1.0", prop="value2")
+        self.assertEqual(states.prop[-1], "value2")
+        self.assertEqual(states.prop.shape, (6,))
+        states.append(T=1100, P=3e5, X="AR:1.0", prop=100)
+        # NumPy converts to the existing type of the array
+        self.assertEqual(states.prop[-1], "100")
+        self.assertEqual(states.prop.shape, (7,))
+
+    def test_append_failures(self):
+        states = ct.SolutionArray(self.gas, 5, extra={"prop": "value"})
+        states.TPX = np.linspace(500, 1000, 5), 2e5, 'H2:0.5, O2:0.4'
+        self.assertEqual(states._shape, (5,))
+
+        with self.assertRaisesRegex(TypeError, "Missing keyword arguments for extra"):
+            states.append(T=1100, P=3e5, X="AR:1.0")
+        # Failing to append a state shouldn't change the size
+        self.assertEqual(states._shape, (5,))
+
+        with self.assertRaisesRegex(KeyError, "does not specify"):
+            # I is not a valid property
+            states.append(TPI=(1100, 3e5, "AR:1.0"), prop="value2")
+        # Failing to append a state shouldn't change the size
+        self.assertEqual(states._shape, (5,))
+
+        with self.assertRaisesRegex(KeyError, "is not a valid"):
+            # I is not a valid property
+            states.append(T=1100, P=3e5, I="AR:1.0", prop="value2")
+        # Failing to append a state shouldn't change the size
+        self.assertEqual(states._shape, (5,))
+
     def test_purefluid(self):
         water = ct.Water()
         states = ct.SolutionArray(water, 5)