Skip to content

Save case sensitive species names #689

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.

Sign up for GitHub

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

Merged
speth merged 6 commits into from
Sep 24, 2019
Merged

Save case sensitive species names #689

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2c33b9
[Thermo] fix compiler warning for RedlichKwongMFTP
Aug 12, 2019
84cbab3
[Thermo] add flag that makes species names case sensitive
Aug 11, 2019
cacffb7
[Thermo] update unit tests to handle case sensitive species names
ischoegl Aug 12, 2019
919cadb
Add case-sensitive/lowercase logic to Phase::species
ischoegl Sep 19, 2019
55e7a30
Isolate lowercase fallback in speciesIndex
ischoegl Sep 23, 2019
fb9c749
Implement faster fallback for non-canonical/lowercase name lookup
ischoegl Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion include/cantera/thermo/Phase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

// This file is part of Cantera. See License.txt in the top-level directory or
// at http://www.cantera.org/license.txt for license and copyright information.
// at https://cantera.org/license.txt for license and copyright information.

#ifndef CT_PHASE_H
#define CT_PHASE_H
Expand Down Expand Up @@ -747,6 +747,17 @@ class Phase
//! change in state is detected
virtual void invalidateCache();

//! Returns `true` if case sensitive species names are enforced
bool caseSensitiveSpecies() const {
return m_caseSensitiveSpecies;
}

//! Set flag that determines whether case sensitive species are enforced
//! in look-up operations, e.g. speciesIndex
void setCaseSensitiveSpecies(bool cflag = true) {
m_caseSensitiveSpecies = cflag;
}

protected:
//! Cached for saved calculations within each ThermoPhase.
/*!
Expand Down Expand Up @@ -794,7 +805,15 @@ class Phase
//! Flag determining behavior when adding species with an undefined element
UndefElement::behavior m_undefinedElementBehavior;

//! Flag determining whether case sensitive species names are enforced
bool m_caseSensitiveSpecies;

private:
//! Find lowercase species name in m_speciesIndices when case sensitive
//! species names are not enforced and a user specifies a non-canonical
//! species name. Raise exception if lowercase name is not unique.
size_t findSpeciesLower(const std::string& nameStr) const;

XML_Node* m_xml; //!< XML node containing the XML info for this phase

//! ID of the phase. This is the value of the ID attribute of the XML
Expand Down
3 changes: 2 additions & 1 deletion interfaces/cython/cantera/_cantera.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,12 +165,13 @@ cdef extern from "cantera/thermo/ThermoPhase.h" namespace "Cantera":

# species properties
size_t nSpecies()
shared_ptr[CxxSpecies] species(string) except +translate_exception
shared_ptr[CxxSpecies] species(size_t) except +translate_exception
size_t speciesIndex(string) except +translate_exception
string speciesName(size_t) except +translate_exception
double nAtoms(size_t, size_t) except +translate_exception
void getAtoms(size_t, double*) except +translate_exception
cbool caseSensitiveSpecies()
void setCaseSensitiveSpecies(cbool)

double molecularWeight(size_t) except +translate_exception
double meanMolecularWeight()
Expand Down
29 changes: 29 additions & 0 deletions interfaces/cython/cantera/test/test_thermo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,6 +1253,35 @@ def test_stringify_bad(self):
with self.assertRaises(AttributeError):
ct.Solution(3)

def test_case_sensitive_names(self):
gas = ct.Solution('h2o2.xml')
self.assertFalse(gas.case_sensitive_species_names)
self.assertTrue(gas.species_index('h2') == 0)

gas.case_sensitive_species_names = True
self.assertTrue(gas.case_sensitive_species_names)
with self.assertRaises(ValueError):
gas.species_index('h2')
with self.assertRaisesRegex(ct.CanteraError, 'Unknown species'):
gas.X = 'h2:1.0, o2:1.0'
with self.assertRaisesRegex(ct.CanteraError, 'Unknown species'):
gas.Y = 'h2:1.0, o2:1.0'

gas_cti = """ideal_gas(
name="gas",
elements=" S C Cs ",
species=" nasa: all ",
options=["skip_undeclared_elements"],
initial_state=state(temperature=300, pressure=(1, "bar"))
)"""
ct.suppress_thermo_warnings(True)
gas = ct.Solution(source=gas_cti)
with self.assertRaisesRegex(ct.CanteraError, 'is not unique'):
gas.species_index('cs')
gas.case_sensitive_species_names = True
with self.assertRaises(ValueError):
gas.species_index('cs')


class TestElement(utilities.CanteraTest):
@classmethod
Expand Down
12 changes: 10 additions & 2 deletions interfaces/cython/cantera/thermo.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# This file is part of Cantera. See License.txt in the top-level directory or
# at http://www.cantera.org/license.txt for license and copyright information.
# at https://cantera.org/license.txt for license and copyright information.

import warnings
import weakref
Expand Down Expand Up @@ -470,6 +470,13 @@ cdef class ThermoPhase(_SolutionBase):

return index

property case_sensitive_species_names:
"""Enforce case-sensitivity for look up of species names"""
def __get__(self):
return self.thermo.caseSensitiveSpecies()
def __set__(self, val):
self.thermo.setCaseSensitiveSpecies(bool(val))

def species(self, k=None):
"""
Return the `Species` object for species *k*, where *k* is either the
Expand All @@ -483,7 +490,8 @@ cdef class ThermoPhase(_SolutionBase):

s = Species(init=False)
if isinstance(k, (str, bytes)):
s._assign(self.thermo.species(stringify(k)))
kk = self.thermo.speciesIndex(stringify(k))
s._assign(self.thermo.species(<int>kk))
Comment thread
ischoegl marked this conversation as resolved.
Outdated
elif isinstance(k, (int, float)):
s._assign(self.thermo.species(<int>k))
else:
Expand Down
108 changes: 87 additions & 21 deletions src/thermo/Phase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

// This file is part of Cantera. See License.txt in the top-level directory or
// at http://www.cantera.org/license.txt for license and copyright information.
// at https://cantera.org/license.txt for license and copyright information.

#include "cantera/thermo/Phase.h"
#include "cantera/base/utilities.h"
Expand All @@ -21,6 +21,7 @@ Phase::Phase() :
m_kk(0),
m_ndim(3),
m_undefinedElementBehavior(UndefElement::add),
m_caseSensitiveSpecies(false),
m_xml(new XML_Node("phase")),
m_id("<phase>"),
m_temp(0.001),
Expand Down Expand Up @@ -172,20 +173,49 @@ void Phase::getAtoms(size_t k, double* atomArray) const
}
}

size_t Phase::findSpeciesLower(const std::string& name) const
{
size_t loc = npos;
std::string nLower = toLowerCopy(name);
for (const auto& k : m_speciesIndices) {
if (toLowerCopy(k.first) == nLower) {
if (loc == npos) {
loc = k.second;
} else {
Copy link
Copy Markdown
Member

@speth speth Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than falling back to an O(N) search anytime a non-canonical species name is provided, I wonder if the following might be a cleaner solution:

  • Add a map of lower-case species names to species indices (say, m_speciesIndicesLower)
  • When adding species, if there is already an entry in this map with the same name, set its value to npos, and possibly set the m_caseSensitiveSpecies flag.
  • The implementation of speciesIndex can then use either m_speciesIndicesLower or m_speciesIndices depending on the value of m_caseSensitiveSpecies. If m_speciesIndicesLower is checked and contains npos, raise the exception about non-unique species identifiers.

Copy link
Copy Markdown
Member Author

@ischoegl ischoegl Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this suggestion. The second (lower-case) species map would contain nearly identical information; in cases like this, I usually keep the information in a single representation, as it is easier to maintain.

While the suggested implementation would be faster if a user opts to use species names with names other than those specified in the input file, I doubt that it has significant real-world benefits. Assuming that those searches are typically done during problem setup, there is little computational overhead (in repeated operations, it is always faster to select species by index). I am also unsure about auto-magically flipping flags, as those decisions may be best left to the user? Weighing cost and benefit, I am suggesting to leave the implementation as is.

Copy link
Copy Markdown
Member

@speth speth Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, there is a benefit to not introducing a new data structure. However, avoiding that requires several instances of this kind of convoluted try/catch construct (with I think another one needed in Phase::species(string).

I think using the extra data structure would allow us to avoid adding findSpeciesLower as a new method -- its logic could just be in speciesIndex.

There's also some value in not generating exceptions for circumstances that aren't really exceptional (at least in C++). I'm less concerned about the performance implication than with the ability to run in gdb and break at an error by simply doing catch throw. If we use exceptions in places like this, then you end up spending a lot more time continuing past these unexceptional exceptions.

I would also note that speciesIndex is actually called with a name not in m_speciesIndices is more often than you might think -- it's not just instances of case-mismatched species names. It is used extensively in multiphase systems as a way of finding which phase a species belongs to. See for example Kinetics::kineticsSpeciesIndex and Kinetics::speciesPhase.

My thought on automatically enabling case sensitivity when there are species that can only be resolved this way was that that might be less surprising to the user than having to enable it explicitly. There may also be issues with initializing phases with species that are only distinguishable by case. For instance, what if a reaction is specified (in the input file) that contains one of these species? Adding the reaction requires looking up the indices of the involved species, and this needs to happen before the user has the chance to set the case sensitive species flag.

Copy link
Copy Markdown
Member Author

@ischoegl ischoegl Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@speth ... thanks for your comments (plus catching the other access-species-by-name case for Phase::species). I.e. this involves another (almost) duplicate map.

Your comment regarding C++ exceptions makes a lot of sense. In the current revision, I opted to replace std::map::at by std::map::find, which eliminates the try - catch blocks.

I do realize that the kinetics managers make extensive use of speciesIndex (they have to), but in most cases the logic will employ case-sensitive species without additional overhead.

Regarding the automatic flags: my thought was that in most cases, case sensitive species are used by default internally and the fallback solution is rarely used. I updated the error message to explicitly direct users towards the caseSensitiveSpecies flag, which can be employed after all species and reactions are set up: internally, everything is case sensitive, i.e. lowercase names are treated as fall-back solutions.

Finally, at this point the case-sensitive/lowercase logic is exclusively handled within Phase::speciesIndex and Phase::species, i.e. there is no longer any duplication of logic blocks.

Copy link
Copy Markdown
Member

@speth speth Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the "automatic flags", I had to play around with this a bit to realize how this was now working - an exception is only thrown when case sensitivity is disabled and you specify a name which is not a case-sensitive match for any species. So my concern about setting up reactions is moot, and I agree that there is no need to automatically toggle this flag.

I still think the speciesIndex calls within the Kinetics managers for interface phases will frequently incur the O(N) overhead. For example, if you have a kinetics manager containing a surface, a gas, and a solid phase in that order, looking up the index of a species in the solid phase, even with the correct case, will cause a search through all the surface phase species, then all the gas phase species, before finally taking the easy path for the index into the solid phase.

throw CanteraError("Phase::findSpeciesLower",
"Lowercase species name '{}' is not unique. "
"Set Phase::caseSensitiveSpecies to true to "
"enforce case sensitive species names", nLower);
}
}
}
return loc;
}

size_t Phase::speciesIndex(const std::string& nameStr) const
{
size_t loc = getValue(m_speciesIndices, toLowerCopy(nameStr), npos);
size_t loc = npos;
std::map<std::string, size_t>::const_iterator it;

it = m_speciesIndices.find(nameStr);
if (it != m_speciesIndices.end()) {
Copy link
Copy Markdown
Member

@speth speth Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good place to use auto instead of writing out the complex iterator type.

Copy link
Copy Markdown
Member Author

@ischoegl ischoegl Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ ... didn't know that one.

return it->second;
} else if (!m_caseSensitiveSpecies) {
loc = findSpeciesLower(nameStr);
}
if (loc == npos && nameStr.find(':') != npos) {
std::string pn;
std::string sn = toLowerCopy(parseSpeciesName(nameStr, pn));
std::string sn = parseSpeciesName(nameStr, pn);
if (pn == "" || pn == m_name || pn == m_id) {
return getValue(m_speciesIndices, sn, npos);
} else {
return npos;
it = m_speciesIndices.find(nameStr);
if (it != m_speciesIndices.end()) {
return it->second;
} else if (!m_caseSensitiveSpecies) {
return findSpeciesLower(sn);
}
}
} else {
return loc;
}
return loc;
}

string Phase::speciesName(size_t k) const
Expand Down Expand Up @@ -292,10 +322,12 @@ void Phase::setMoleFractions_NoNorm(const doublereal* const x)
void Phase::setMoleFractionsByName(const compositionMap& xMap)
{
vector_fp mf(m_kk, 0.0);

for (const auto& sp : xMap) {
try {
mf[m_speciesIndices.at(toLowerCopy(sp.first))] = sp.second;
} catch (std::out_of_range&) {
size_t loc = speciesIndex(sp.first);
if (loc != npos) {
mf[loc] = sp.second;
} else {
throw CanteraError("Phase::setMoleFractionsByName",
"Unknown species '{}'", sp.first);
}
Expand Down Expand Up @@ -338,10 +370,18 @@ void Phase::setMassFractionsByName(const compositionMap& yMap)
vector_fp mf(m_kk, 0.0);
for (const auto& sp : yMap) {
try {
mf[m_speciesIndices.at(toLowerCopy(sp.first))] = sp.second;
mf[m_speciesIndices.at(sp.first)] = sp.second;
} catch (std::out_of_range&) {
throw CanteraError("Phase::setMassFractionsByName",
"Unknown species '{}'", sp.first);
size_t loc = npos;
if (!m_caseSensitiveSpecies) {
loc = findSpeciesLower(sp.first);
Comment thread
ischoegl marked this conversation as resolved.
Outdated
}
if (loc == npos) {
throw CanteraError("Phase::setMassFractionsByName",
"Unknown species '{}'", sp.first);
} else {
mf[loc] = sp.second;
}
}
}
setMassFractions(&mf[0]);
Expand Down Expand Up @@ -699,7 +739,7 @@ size_t Phase::addElement(const std::string& symbol, doublereal weight,
}

bool Phase::addSpecies(shared_ptr<Species> spec) {
if (m_species.find(toLowerCopy(spec->name)) != m_species.end()) {
if (m_species.find(spec->name) != m_species.end()) {
Comment thread
ischoegl marked this conversation as resolved.
throw CanteraError("Phase::addSpecies",
"Phase '{}' already contains a species named '{}'.",
m_name, spec->name);
Expand Down Expand Up @@ -729,8 +769,8 @@ bool Phase::addSpecies(shared_ptr<Species> spec) {
}

m_speciesNames.push_back(spec->name);
m_species[toLowerCopy(spec->name)] = spec;
m_speciesIndices[toLowerCopy(spec->name)] = m_kk;
m_species[spec->name] = spec;
m_speciesIndices[spec->name] = m_kk;
m_speciesCharge.push_back(spec->charge);
size_t ne = nElements();

Expand Down Expand Up @@ -794,24 +834,50 @@ void Phase::modifySpecies(size_t k, shared_ptr<Species> spec)
"New species name '{}' does not match existing name '{}'",
spec->name, speciesName(k));
}
const shared_ptr<Species>& old = m_species[toLowerCopy(spec->name)];
const shared_ptr<Species>& old = m_species[spec->name];
if (spec->composition != old->composition) {
throw CanteraError("Phase::modifySpecies",
"New composition for '{}' does not match existing composition",
spec->name);
}
m_species[toLowerCopy(spec->name)] = spec;
m_species[spec->name] = spec;
invalidateCache();
}

shared_ptr<Species> Phase::species(const std::string& name) const
{
return m_species.at(toLowerCopy(name));
std::map<std::string, shared_ptr<Species> >::const_iterator it;

it = m_species.find(name);
if (it != m_species.end()) {
return it->second;
} else if (!m_caseSensitiveSpecies) {
std::string nLower = toLowerCopy(name);
bool found = false;
shared_ptr<Species> sp;
for (const auto& k : m_species) {
if (toLowerCopy(k.first) == nLower) {
if (!found) {
found = true;
sp = k.second;
} else {
throw CanteraError("Phase::species",
"Lowercase species name '{}' is not unique. "
"Set Phase::caseSensitiveSpecies to true to "
"enforce case sensitive species names", nLower);
}
}
}
return sp;
} else {
throw CanteraError("Phase::setMassFractionsByName",
"Unknown species '{}'", name);
}
Comment thread
ischoegl marked this conversation as resolved.
}

shared_ptr<Species> Phase::species(size_t k) const
{
return species(m_speciesNames[k]);
return m_species.at(m_speciesNames[k]);
}

void Phase::ignoreUndefinedElements() {
Expand Down
11 changes: 9 additions & 2 deletions src/thermo/RedlichKwongMFTP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! @file RedlichKwongMFTP.cpp

// This file is part of Cantera. See License.txt in the top-level directory or
// at http://www.cantera.org/license.txt for license and copyright information.
// at https://cantera.org/license.txt for license and copyright information.

#include "cantera/thermo/RedlichKwongMFTP.h"
#include "cantera/thermo/ThermoFactory.h"
Expand Down Expand Up @@ -718,7 +718,8 @@ vector<double> RedlichKwongMFTP::getCoeff(const std::string& iName)
if (iNameLower == dbName) {
// Read from database and calculate a and b coefficients
double vParams;
double T_crit, P_crit;
double T_crit=0.;
double P_crit=0.;

if (acNodeDoc.hasChild("Tc")) {
vParams = 0.0;
Expand All @@ -734,6 +735,9 @@ vector<double> RedlichKwongMFTP::getCoeff(const std::string& iName)
"Critical Temperature must be positive ");
}
T_crit = vParams;
} else {
throw CanteraError("RedlichKwongMFTP::GetCoeff",
"Critical Temperature not in database ");
}
if (acNodeDoc.hasChild("Pc")) {
vParams = 0.0;
Expand All @@ -749,6 +753,9 @@ vector<double> RedlichKwongMFTP::getCoeff(const std::string& iName)
"Critical Pressure must be positive ");
}
P_crit = vParams;
} else {
throw CanteraError("RedlichKwongMFTP::GetCoeff",
"Critical Pressure not in database ");
}

//Assuming no temperature dependence
Expand Down
2 changes: 1 addition & 1 deletion src/transport/GasTransport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ void GasTransport::setupCollisionIntegral()
void GasTransport::getTransportData()
{
for (size_t k = 0; k < m_thermo->nSpecies(); k++) {
shared_ptr<Species> s = m_thermo->species(m_thermo->speciesName(k));
shared_ptr<Species> s = m_thermo->species(k);
const GasTransportData* sptran =
dynamic_cast<GasTransportData*>(s->transport.get());
if (!sptran) {
Expand Down