Skip to content
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.

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

Prevent segfaults for partial 0-D objects #1661

Merged
merged 4 commits into from
Jan 20, 2024
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jan 20, 2024

Changes proposed in this pull request

  • Prevent segfaults in reactors with no contents
  • Prevent segfaults for unconnected walls
  • Add google tests

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

  • 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

Copy link

codecov bot commented Jan 20, 2024

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 ⚠️
src/zeroD/Wall.cpp 83.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            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     

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

@ischoegl ischoegl marked this pull request as ready for review January 20, 2024 16:49
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl, all the changes here look good to me, and I appreciate the thoughts on longer-term options to eliminate the need for these checks.

My only request here is to document what upstream deprecation warning is resolved by installing pyarrow as part of the commit message. Do we need to update the dependencies of some of the examples?

Prevent the following deprecation warning from breaking the CI run:
"Pyarrow will become a required dependency of pandas in the next major
release of pandas (pandas 3.0)"
@ischoegl
Copy link
Member Author

ischoegl commented Jan 20, 2024

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

👍 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.

@speth speth merged commit 8d676c0 into Cantera:main Jan 20, 2024
48 of 49 checks passed
@ischoegl ischoegl deleted the fix-1623 branch January 21, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access violation dealing with Wall between empty Reactors
2 participants