Add speed of sound property for ThermoPhase by corykinney · Pull Request #1491 · Cantera/cantera

corykinney · 2023-05-17T20:33:42Z

Changes proposed in this pull request

Add ThermoPhase::soundVelocity virtual function that returns the equilibrium speed of sound in m/s.
Add finite difference consistency test c_eq_sqrt_dP_drho_const_s
Implement definitions for IdealGasPhase, RedlichKwongMFTP, and PengRobinson

Marking this as a draft to see if this would be useful or not. I'm also not sure whether soundVelocity is the best name, I also considered soundSpeed and equilibriumSoundSpeed, or something else could be better. I'm also not certain if ThermoPhase is the best place for it, and I'm not positive I added it to the cython interface properly.

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

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

bryanwweber · 2023-05-18T15:51:28Z

I have a preference for equilibriumSoundSpeed and equilibrium_sound_speed for C++ and Python, respectively.

speth · 2023-05-18T16:58:23Z

Thanks, @corykinney. I think that adding this property is useful, especially when accompanied with implementations for phases other than the ideal gas model, where it's a non-trivial calculation.

I agree that "speed" is preferable to "velocity", but I think calling this the "equilibrium" sound speed would be a misnomer, since the speed is not being computed while holding the mixture at chemical equilibrium. We even have an example illustrating this difference (sound_speed.py).

corykinney · 2023-05-19T15:13:11Z

Thanks, @corykinney. I think that adding this property is useful, especially when accompanied with implementations for phases other than the ideal gas model, where it's a non-trivial calculation.

I agree that "speed" is preferable to "velocity", but I think calling this the "equilibrium" sound speed would be a misnomer, since the speed is not being computed while holding the mixture at chemical equilibrium. We even have an example illustrating this difference (sound_speed.py).

Yes, frozen was the term I was looking for, not equilibrium. I can rename them to soundSpeed and sound_speed for C++ and Python, respectively.

ischoegl

@corykinney ... thanks for the PR. Before a more in-depth review, could you please look into the failing tests? Most likely, you need to (i) add sound_speed to the list of _scalar definitions within the SolutionArray class in composite.py (around line 585), and (ii) add sound_speed to the scalar property _attr list in onedim.py (around line 770).

codecov · 2023-06-12T23:33:07Z

Codecov Report

Merging #1491 (ecdac2a) into main (5973dbd) will increase coverage by 0.53%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #1491      +/-   ##
==========================================
+ Coverage   69.76%   70.30%   +0.53%     
==========================================
  Files         377      376       -1     
  Lines       58017    58261     +244     
  Branches    20701    20804     +103     
==========================================
+ Hits        40477    40960     +483     
+ Misses      14585    14316     -269     
- Partials     2955     2985      +30

Impacted Files	Coverage Δ
include/cantera/thermo/IdealGasPhase.h	`89.74% <ø> (ø)`
include/cantera/thermo/PengRobinson.h	`100.00% <ø> (ø)`
include/cantera/thermo/RedlichKwongMFTP.h	`100.00% <ø> (ø)`
interfaces/cython/cantera/composite.py	`83.38% <ø> (ø)`
interfaces/cython/cantera/onedim.py	`83.54% <ø> (ø)`
include/cantera/thermo/ThermoPhase.h	`46.87% <50.00%> (+0.03%)`	⬆️
interfaces/cython/cantera/thermo.pyx	`93.18% <100.00%> (+<0.01%)`	⬆️
src/thermo/IdealGasPhase.cpp	`92.59% <100.00%> (+0.18%)`	⬆️
src/thermo/PengRobinson.cpp	`84.80% <100.00%> (+0.09%)`	⬆️
src/thermo/RedlichKwongMFTP.cpp	`84.49% <100.00%> (+0.08%)`	⬆️

... and 24 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

corykinney · 2023-06-13T00:38:44Z

@corykinney ... thanks for the PR. Before a more in-depth review, could you please look into the failing tests? Most likely, you need to (i) add sound_speed to the list of _scalar definitions within the SolutionArray class in composite.py (around line 585), and (ii) add sound_speed to the scalar property _attr list in onedim.py (around line 770).

Looks like that fixed the failing checks!

ischoegl

Hi @corykinney ... thanks for fixing CI - I'm happy that this did the trick! Changes look mostly good to me.

The only other thing I'd ask you to update is the Python example sound_speed.py so it uses the new built-in function for the check with afrozen2. Also, please make sure to update the requires in the header.

corykinney · 2023-06-13T17:22:07Z

@ischoegl I updated the example as requested (assuming this change will make it into 3.0.0). Also, I saw that there was a sound_speed_units.py example as well, so I believe I implemented the units for sound_speed as required, but I'd like to get a second set of eyes on it.

ischoegl

Thanks, @corykinney ... this looks good from my side. I'll leave it open for a couple of days in case anyone else has anything to add.

Merge conflict.

ischoegl · 2023-06-13T21:56:10Z

@corykinney ... it looks like there now is a merge conflict due to recent upstream changes (#1501 moved consistency tests to a separate test). Could you rebase?

corykinney · 2023-06-14T14:47:15Z

@corykinney ... it looks like there now is a merge conflict due to recent upstream changes (#1501 moved consistency tests to a separate test). Could you rebase?

@ischoegl Just force pushed with the rebased commits

speth

Thanks, @corykinney, and my apologies for introducing the merge conflict with this. I had just one small suggestion before this is merged.

include/cantera/thermo/IdealGasPhase.h

speth · 2023-06-14T16:30:00Z

Thanks for the quick update, @corykinney. However, I think your commits are now out of order, such that building from some of the intermediate ones will fail. I think it should be:

Add virtual method to ThermoPhase and implementation for IdealGasPhase
Add the implementations for P-R and R-K
Add the consistency test
Update the example

Add ThermoPhase::soundSpeed virtual function with documentation Add IdealGasPhase implementation Add to cython interface Add units declaration

Add definitions of soundSpeed RedlichKwongMFTP and PengRobinson classes

Add finite difference check for soundSpeed

Updated sound_speed.py and sound_speed_units.py to check against the new built-in function

corykinney · 2023-06-14T17:03:13Z

Thanks for the quick update, @corykinney. However, I think your commits are now out of order, such that building from some of the intermediate ones will fail. I think it should be:
1. Add virtual method to `ThermoPhase` and implementation for `IdealGasPhase`

2. Add the implementations for P-R and R-K

3. Add the consistency test

4. Update the example

@speth The changes should be grouped and ordered properly now. Thanks for your patience! Let me know if there is anything else.

speth

Thanks for taking care of these details, @corykinney!

corykinney force-pushed the sound-velocity branch from 63d4330 to 2209329 Compare May 19, 2023 16:40

corykinney marked this pull request as ready for review May 19, 2023 16:41

ischoegl added the Thermo label May 22, 2023

ischoegl reviewed Jun 1, 2023

View reviewed changes

ischoegl requested changes Jun 13, 2023

View reviewed changes

ischoegl previously approved these changes Jun 13, 2023

View reviewed changes

corykinney force-pushed the sound-velocity branch from 4e5de23 to 852d1f9 Compare June 14, 2023 14:46

speth requested changes Jun 14, 2023

View reviewed changes

include/cantera/thermo/IdealGasPhase.h Outdated Show resolved Hide resolved

corykinney force-pushed the sound-velocity branch from 852d1f9 to 3035131 Compare June 14, 2023 16:20

corykinney added 4 commits June 14, 2023 12:59

[Thermo] Add speed of sound property

20528f4

Add ThermoPhase::soundSpeed virtual function with documentation Add IdealGasPhase implementation Add to cython interface Add units declaration

[Thermo] Add real gas speed of sound definitions

bd20a31

Add definitions of soundSpeed RedlichKwongMFTP and PengRobinson classes

[Test] Add thermo consistency test for speed of sound

182ba8d

Add finite difference check for soundSpeed

[Examples] Update speed of sound examples

ecdac2a

Updated sound_speed.py and sound_speed_units.py to check against the new built-in function

corykinney force-pushed the sound-velocity branch from 3035131 to ecdac2a Compare June 14, 2023 16:59

speth approved these changes Jun 14, 2023

View reviewed changes

speth merged commit 931089b into Cantera:main Jun 14, 2023

corykinney deleted the sound-velocity branch June 14, 2023 20:55

Uh oh!

Conversation

corykinney commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented May 18, 2023

Uh oh!

speth commented May 18, 2023

Uh oh!

corykinney commented May 19, 2023

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

corykinney commented Jun 13, 2023

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

corykinney commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

corykinney commented Jun 14, 2023

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

speth commented Jun 14, 2023

Uh oh!

corykinney commented Jun 14, 2023

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

corykinney commented May 17, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

corykinney commented Jun 13, 2023 •

edited

Loading

ischoegl commented Jun 13, 2023 •

edited

Loading