Improve error message for missing phases key

[ischoegl]

Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• The pull request is ready for review

If applicable, fill in the issue number this pull request is fixing

Fixes #812

Changes proposed in this pull request

• Cython CxxAnyValue& operator[](string) uses the operator version that adds a key if it is not found (instead of the const version that raises an error in C++)
• Use AnyMap::at instead (Cython support for C++ const Is incomplete)
• Pre-emptively replace operator [] by AnyMap::at in other instances

The error now points to the actual problem, i.e.

In [1]: import cantera as ct

In [2]: yaml = '''
   ...:         species:
   ...:         - name: H
   ...:           composition: {H: 1}
   ...:           thermo:
   ...:             model: NASA7
   ...:             temperature-ranges: [200.0, 1000.0, 6000.0]
   ...:             data:
   ...:             - [2.5, 0.0, 0.0, 0.0, 0.0, 2.547366e+04, -0.44668285]
   ...:             - [2.5, 0.0, 0.0, 0.0, 0.0, 2.547366e+04, -0.44668285]
   ...:             note: L6/94
   ...:         '''

In [3]: ct.Solution(yaml=yaml)
---------------------------------------------------------------------------
CanteraError                              Traceback (most recent call last)
<ipython-input-11-496cc576f3d1> in <module>()
----> 1 ct.Solution(yaml=yaml)

interfaces/cython/cantera/base.pyx in cantera._cantera._SolutionBase.__cinit__()

interfaces/cython/cantera/base.pyx in cantera._cantera._SolutionBase._init_yaml()

CanteraError: 
***********************************************************************
InputFileError thrown by AnyMap::at:
Error on line 2 of input string:
Key 'phases' not found.
Existing keys: species
|  Line |
|     1 | 
>     2 >         species:
                  ^
|     3 |         - name: H
|     4 |           composition: {H: 1}
|     5 |           thermo:
***********************************************************************

[codecov]

Codecov Report

Merging #880 into master will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@           Coverage Diff           @@
##           master     #880   +/-   ##
=======================================
  Coverage   70.49%   70.50%           
=======================================
  Files         375      375           
  Lines       45649    45685   +36     
=======================================
+ Hits        32182    32210   +28     
- Misses      13467    13475    +8     

Impacted Files	Coverage Δ
src/base/AnyMap.cpp	83.78% <50.00%> (-0.06%)	⬇️
test/general/test_containers.cpp	100.00% <100.00%> (ø)
src/kinetics/KineticsFactory.cpp	96.38% <0.00%> (-1.12%)	⬇️
src/kinetics/Reaction.cpp	87.12% <0.00%> (-0.38%)	⬇️
src/kinetics/Kinetics.cpp	83.85% <0.00%> (-0.36%)	⬇️
include/cantera/base/AnyMap.h	100.00% <0.00%> (ø)

---

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 df37795...bd8ffd6. Read the comment docs.

[speth]

I think the changes here look good as far as fixing this for the Python module goes. Could you make the corresponding change for the C++ interface (in the newPhase function)?

[ischoegl]

@speth ... oh, for some reason I had assumed that this was Python specific. I guess the same rationale applies in C++ when you call a method on something returned by the [] operator?

[speth]

Yes, and I suspect the fix is as simple as what you did here (or simpler, since you won't get into a fight with proper const support).

[ischoegl]

@speth ... I verified that the same error occurs in C++. So far, so good; after adding a non-const version of AnyMap::at I can apply the same fix as in Cython.

However, similar errors can occur for every single instance where the [] operator is used on the right-hand side (as far as I can tell, the obvious intended use for the non-const version is for creation of a new field on the left-hand side). There are numerous instances (based on searching for ]. instances) for RHS uses. I see two options:

1. Eliminate all RHS uses of the operator []
2. Update the error logic for AnyMap::getMapWhere (and other methods) to catch and replace the misleading error. It looks like you'd have to check whether the AnyMap instance is empty.
3. Apply both, especially where (2) may have an error message that is insufficiently clear.

[speth]

Yes, you're right, there are many places where this can occur. I think option (1) would be annoying to police, so I'd rather avoid that. Option (2) is actually not too difficult for AnyMap::getMapWhere -- it's just one additional check in the if...else chains that looks like:

    } else if (is<void>()) {
        throw InputFileError("AnyValue::getMapWhere", *this,
            "Key '{}' not found", m_key);

We already do a similar check in AnyValue::as, which is the other case that came to mind where this would be an issue, and these two might actually be the end of it.

With this change, I'm wondering whether any of the other changes are even necessary.

[ischoegl]

@speth ... this is indeed a workable solution! It throws almost the exact same error (minus the list of available keys).

I'll update accordingly.

[ischoegl]

@speth ... done (undid most of the changes, so the PR is much simpler now).

PS: commit bd8ffd6 may be superseded by changes in #883 ...

PPS: the only difference is that empty fields are now created, but I don't think that this matters much.

[speth]

Looks good. I think the fix for the test files that needed to be added to .gitignore is good - this was the result of changes made in #803, which resulted in the names of some of the regression test executables changing. I didn't notice it at the time because I made those changes on a Windows machine, where all of the test executables are excluded by the *.exe filter in test_problems/.gitignore.

[ischoegl]

@speth ... didn't realized that there was a separate test_problems/.gitignore until you mentioned it. I pushed #888 to make this more consistent (I also decluttered and alphabetized).