Make index lookups more robust by ischoegl · Pull Request #2010 · Cantera/cantera

ischoegl · 2025-10-13T19:14:48Z

Changes proposed in this pull request

This PR aims to establish a more uniform approach to index lookups, where the current npos check (utilized for internal methods) is transitioned towards standard exception handling (suitable for user-facing APIs).

Mimic the implementation of the clone flag in Clone Solution objects used by Reactor / ReactorSurface #1921 ... a raise flag is currently set to false and will switch to a true default in Cantera 3.2. This is implemented for various index lookups in Phase, MultiPhase, and Kinetics.
Make more widespread use of checkABCIndex
Deprecate some additional legacy CLib helper functions
Applies to indexing of Element, Species, Reaction, and Component.
Immediately switch Reactor::componentIndex/Reactor::speciesIndex to throw exceptions: almost all uses are in user-facing APIs (and a transition for ExtensibleXYZReactor would be a breaking change).
Make exception messages more consistent.

One major advantage of this approach is that exception handling at user-facing APIs will become significantly easier, e.g., in #1997 (the MATLAB API itself is still hand-coded, but other APIs, such as .NET, would require cumbersome workarounds).

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

Closes #2004

If applicable, provide an example illustrating new features this pull request is introducing

Historically, exception handling was separated into three functions, although the checkABC was rarely used, and checkABCArraySize, which isn't directly related, only appeared in the CLib interface. For Phase::speciesIndex, this looks as follows:

//! Returns the index of a species named 'name' within the Phase object.
size_t speciesIndex(const string& name) const;

//! Check that the specified species index is in range.
bool checkSpeciesIndex(size_t k) const;

//! Check that an array size is at least nSpecies().
void checkSpeciesArraySize(size_t kk) const;

This PR proposes to integrate exception handling into speciesIndex.

//! Returns the index of a species named 'name' within the Phase object.
size_t speciesIndex(const string& name, bool raise) const;

After Cantera 3.2, this PR anticipates a default argument of bool raise=true.

checkSpeciesIndex is still useful as an internal function (making it protected would be within the scope of this PR), and is updated to return the correct size as:

//! Check that the specified species index is in range.
size_t checkSpeciesIndex(size_t k) const;

Regarding checkABCArraySize, the generated CLib goes a different route, where size checking is enabled by the uses field of the YAML specification. This could be further streamlined if Cantera/enhancements#237 were to be implemented.

As an aside, the Python API widely uses a ThermoPhase.species_index(int) et al. to validate indices, which unnecessarily complicates the interface, but remains intact.

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2025-10-13T20:52:04Z

Codecov Report

❌ Patch coverage is 71.77700% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.43%. Comparing base (7e21f3e) to head (eb4f211).
⚠️ Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
src/equil/MultiPhase.cpp	26.47%	23 Missing and 2 partials ⚠️
src/kinetics/Kinetics.cpp	52.77%	10 Missing and 7 partials ⚠️
src/thermo/Phase.cpp	88.37%	3 Missing and 2 partials ⚠️
interfaces/cython/cantera/thermo.pyx	60.00%	4 Missing ⚠️
src/zeroD/IdealGasConstPressureReactor.cpp	20.00%	2 Missing and 2 partials ⚠️
src/kinetics/Reaction.cpp	66.66%	0 Missing and 3 partials ⚠️
src/zeroD/ConstPressureMoleReactor.cpp	25.00%	2 Missing and 1 partial ⚠️
src/zeroD/ConstPressureReactor.cpp	40.00%	2 Missing and 1 partial ⚠️
src/zeroD/IdealGasConstPressureMoleReactor.cpp	50.00%	2 Missing and 1 partial ⚠️
interfaces/cython/cantera/_onedim.pyx	60.00%	2 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2010      +/-   ##
==========================================
+ Coverage   75.18%   75.43%   +0.24%     
==========================================
  Files         454      454              
  Lines       56812    56773      -39     
  Branches     9374     9345      -29     
==========================================
+ Hits        42715    42825     +110     
+ Misses      10919    10785     -134     
+ Partials     3178     3163      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

include/cantera/equil/MultiPhase.h

speth

Thanks, @ischoegl. Overall, I think this is worthwhile, although there are a few rough edges that are worth smoothing out.

interfaces/cython/cantera/thermo.pyx

interfaces/sourcegen/src/sourcegen/headers/ctthermo.yaml

interfaces/cython/cantera/mixture.pyx

include/cantera/equil/MultiPhase.h

include/cantera/kinetics/Kinetics.h

speth · 2025-10-19T01:28:44Z

src/zeroD/ConstPressureMoleReactor.cpp

+    try {
+        return speciesIndex(nm) + m_sidx;
+    } catch (const CanteraError&) {
+        throw CanteraError("ConstPressureMoleReactor::componentIndex",
+            "Component '{}' not found", nm);


I think it would be best to minimize use of this pattern, which could be done by extending the raise argument to ReactorBase::speciesIndex.

This is somewhat tricky. The ExtensibleReactor mechanism boxes us in here significantly, as a change of signatures is non-trivial (need to register a new delegated function type, thread everything through, and I'm not sure that default arguments work).

I parked another way I could think of in this commit ischoegl@b11e2e9 which, for inexplicable reasons, breaks an extensible reactor test also.

Fair enough. I'm OK with this approach. Thanks for investigating the alternatives.

src/zeroD/FlowReactor.cpp

Only raise deprecation warnings if npos or -1 is expected, and the new 'raise' argument is needed.

ischoegl · 2025-10-19T14:30:11Z

@speth ... thanks for the review! The suggestion to condition the deprecation warning was great (#2010 (comment)). I rolled back much of the churn that would have been visible on the user-facing end.

For avoiding try/catch blocks, I am, however, facing some roadblocks that are related to ExtensibleReactor. Modifying the signature of Reactor::speciesIndex is a no-go, and some of the callback mechanism messes up things for the other workaround I could come up with (ischoegl@b11e2e9). I ended up searching for uses of ->componentIndex( and .componentIndex( in 0D .h/.cpp and it only appears to be used internally by ReactorNet::globalComponentIndex, which itself is user-facing. As the pattern appears to be limited to user-facing service methods, I'm suggesting leaving this as is.

As an aside, I added the #pragma warnings for the legacy CLib here: 392ff53 as this is (hopefully!) my last substantial PR before 3.2.

ischoegl · 2025-10-20T12:32:58Z

@speth ... this is ready for a review.

It does contain a (minor) breaking change for the MATLAB PR #1997, where all the indexing methods need to add the raise=true option so the API doesn't need to do the checking (which was part of the motivation here). The second argument can be dropped after Cantera 3.2, i.e., once raise=true becomes the default.

speth · 2025-10-20T13:27:10Z

The second argument can be dropped after Cantera 3.2, i.e., once raise=true becomes the default.

I'm a little confused by the plan here. For CLib, the second argument will always be mandatory, since C doesn't support functions with default arguments.

ischoegl · 2025-10-20T13:35:43Z

The second argument can be dropped after Cantera 3.2, i.e., once raise=true becomes the default.

I'm a little confused by the plan here. For CLib, the second argument will always be mandatory, since C doesn't support functions with default arguments.

True, but in the generated CLib specification, you can force a shorter signature with the wraps field, which then uses default arguments. 😀

speth · 2025-10-20T16:37:03Z

True, but in the generated CLib specification, you can force a shorter signature with the wraps field, which then uses default arguments. 😀

So is the plan for CLib that for Cantera 3.2, you must provide the second argument, and then for Cantera 3.3, you must not, with the behavior being to always indicate an exception for out-of-bounds indices? It might be better to temporarily use a "custom code" implementation for these methods, and avoid requiring downstream changes.

ischoegl · 2025-10-20T16:50:36Z

So is the plan for CLib that for Cantera 3.2, you must provide the second argument, and then for Cantera 3.3, you must not, with the behavior being to always indicate an exception for out-of-bounds indices? It might be better to temporarily use a "custom code" implementation for these methods, and avoid requiring downstream changes.

I guess that's another way. My thought process was that updating MATLAB would be the easiest (the second argument wouldn't be exposed in the user-facing API). But we cannot rule out that someone will adopt CLib and be forced into a change. Easy enough to implement ... in which case this won't be a breaking change to #1997.

ischoegl · 2025-10-20T17:49:13Z

Done. I did, however, realize that there is one more affected method, Kinetics::kineticsSpeciesIndex, that should be updated.

Prevents future changes in the CLib code despite changes in the C++ API.

ischoegl · 2025-10-20T20:27:19Z

@speth ... I had not anticipated using custom code for something like this, but it works quite well 😄. I believe this should be ready to merge now.

speth

Thanks, @ischoegl. I think this is good to go.

speth · 2025-10-20T20:41:06Z

src/zeroD/ConstPressureMoleReactor.cpp

+    try {
+        return speciesIndex(nm) + m_sidx;
+    } catch (const CanteraError&) {
+        throw CanteraError("ConstPressureMoleReactor::componentIndex",
+            "Component '{}' not found", nm);


Fair enough. I'm OK with this approach. Thanks for investigating the alternatives.

ischoegl mentioned this pull request Oct 13, 2025

elementIndex and speciesIndex can cause segfaults #2004

Closed

ischoegl force-pushed the change-index-exceptions branch from 088f4f1 to 50bbe3d Compare October 13, 2025 19:46

ischoegl added the Thermo label Oct 13, 2025

ischoegl marked this pull request as ready for review October 13, 2025 20:17

ischoegl mentioned this pull request Oct 13, 2025

[MATLAB] Migrate to generated Clib #1997

Merged

5 tasks

ischoegl requested a review from a team October 13, 2025 20:40

ischoegl commented Oct 13, 2025

View reviewed changes

include/cantera/equil/MultiPhase.h Outdated Show resolved Hide resolved

ischoegl force-pushed the change-index-exceptions branch 2 times, most recently from d664b4f to 4d8f47f Compare October 17, 2025 21:51

speth requested changes Oct 19, 2025

View reviewed changes

ischoegl added 2 commits October 19, 2025 00:01

[base] Update exception handling for elementIndex

0a90d0e

[base] Make element index checking more consistent

dc2acc9

ischoegl force-pushed the change-index-exceptions branch from 4d8f47f to 2fd32ae Compare October 19, 2025 07:10

ischoegl added 9 commits October 19, 2025 02:19

[base] Improve exception handling for speciesIndex

ae3078c

[base] Make checkSpeciesIndex more useful

0337da8

[base] Make phaseIndex more consistent

7802cc9

[base] Address additional indices/deprecations

5ce2f75

[zeroD] Always raise exception for invalid entries

a441cc0

[base] Make exception messages consistent

80ed52f

[Python] Simplify Sim1D.domain_index

612ce7f

[unittest] Fix legacy CLib unit tests

eaa839c

[mix] Make return type consistent

2d54b13

ischoegl force-pushed the change-index-exceptions branch from 2fd32ae to c2685dc Compare October 19, 2025 07:36

ischoegl added 3 commits October 19, 2025 09:14

Address review comments

3eefaa9

[base] Only issue warnings if <abc>Index behavior changes

01d7e6c

Only raise deprecation warnings if npos or -1 is expected, and the new 'raise' argument is needed.

[legacy CLib] Add pragma deprecation warnings

392ff53

ischoegl force-pushed the change-index-exceptions branch from c2685dc to 392ff53 Compare October 19, 2025 14:16

ischoegl requested a review from speth October 19, 2025 14:51

[Kinetics] Implement raise option for kineticsSpeciesIndex

15910f2

ischoegl force-pushed the change-index-exceptions branch from 5b8932b to 0cd239d Compare October 20, 2025 19:28

[CLib] Implement temporary custom code

eb4f211

Prevents future changes in the CLib code despite changes in the C++ API.

ischoegl force-pushed the change-index-exceptions branch from 0cd239d to eb4f211 Compare October 20, 2025 19:31

speth approved these changes Oct 20, 2025

View reviewed changes

speth merged commit 5e351a1 into Cantera:main Oct 20, 2025
47 of 49 checks passed

ischoegl mentioned this pull request Oct 21, 2025

Improve exceptions for abcName() #2014

Merged

5 tasks

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

speth Oct 19, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

speth Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ischoegl commented Oct 20, 2025

Uh oh!

speth commented Oct 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

speth commented Oct 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ischoegl commented Oct 20, 2025

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

speth Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ischoegl commented Oct 13, 2025 •

edited

Loading

codecov bot commented Oct 13, 2025 •

edited

Loading

ischoegl Oct 19, 2025 •

edited

Loading

ischoegl commented Oct 19, 2025 •

edited

Loading

ischoegl commented Oct 20, 2025 •

edited

Loading

ischoegl commented Oct 20, 2025 •

edited

Loading

ischoegl commented Oct 20, 2025 •

edited

Loading