Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite
https://cantera.org
Other
581 stars 342 forks source link

Prevent segfaults for partial 0-D objects #1661

Closed ischoegl closed 5 months ago

ischoegl commented 5 months ago

Changes proposed in this pull request

Currently existing guards are largely specific to the implementation of the Python API (example: retrieving temperature for an empty reactor fails when attempting to load the current reactor state into a thermo object). As underlying C++ code is difficult to reach from Python (but may be exposed elsewhere), unit tests are added for C++ directly.

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

Fixes #1623

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

Example taken from #1623:

In [1]: import cantera as ct
   ...:
   ...: r1 = ct.Reactor()
   ...: r2 = ct.Reactor()
   ...:
   ...: w = ct.Wall(r1,r2)

In [2]: w.heat_rate
---------------------------------------------------------------------------
CanteraError                              Traceback (most recent call last)
Cell In[2], line 1
----> 1 w.heat_rate

File /Volumes/Data/work/GitHub/cantera/build/python/cantera/reactor.pyx:1066, in cantera.reactor.WallBase.heat_rate.__get__()
   1064 .. versionadded:: 3.0
   1065 """
-> 1066 return self.wall.heatRate()
   1067
   1068

CanteraError:
*******************************************************************************
CanteraError thrown by ReactorBase::temperature:
Reactor state empty and/or contents not defined.
*******************************************************************************

Other thoughts

An alternative to the added guards is to prevent the creation of empty (or unconnected) 0-D objects altogether (which should likely be done when moving from raw pointers to shared pointers for various held objects - m_thermo, etc.). As this is more involved and will require a deprecation cycle, the guards proposed in this PR are still appropriate.

Checklist

codecov[bot] commented 5 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (01a0284) 72.75% compared to head (faddc8f) 72.78%.

Files Patch % Lines
include/cantera/zeroD/ReactorBase.h 58.33% 0 Missing and 5 partials :warning:
src/zeroD/Wall.cpp 83.33% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1661 +/- ## ========================================== + Coverage 72.75% 72.78% +0.02% ========================================== Files 375 375 Lines 56584 56610 +26 Branches 20494 20510 +16 ========================================== + Hits 41168 41201 +33 + Misses 12401 12389 -12 - Partials 3015 3020 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ischoegl commented 5 months ago

My only request here is to document what upstream deprecation warning is resolved by installing pyarrow as part of the commit message.

:+1: done

Do we need to update the dependencies of some of the examples?

I don't think so - it's just pandas complaining that it will require pyarrow down the line.