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

Add speed of sound property for ThermoPhase #1491

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

corykinney
Copy link
Contributor

@corykinney corykinney commented May 17, 2023

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
Copy link
Member

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

@speth
Copy link
Member

speth commented May 18, 2023

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
Copy link
Contributor Author

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.

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

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

@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
Copy link

codecov bot commented Jun 12, 2023

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
Copy link
Contributor Author

@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!

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

corykinney commented Jun 13, 2023

@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
ischoegl previously approved these changes Jun 13, 2023
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

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.

@ischoegl ischoegl dismissed their stale review June 13, 2023 21:55

Merge conflict.

@ischoegl
Copy link
Member

ischoegl commented Jun 13, 2023

@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
Copy link
Contributor Author

@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

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, @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 Outdated Show resolved Hide resolved
@speth
Copy link
Member

speth commented Jun 14, 2023

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

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
Copy link
Contributor Author

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.

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 for taking care of these details, @corykinney!

@speth speth merged commit 931089b into Cantera:main Jun 14, 2023
40 checks passed
@corykinney corykinney deleted the sound-velocity branch June 14, 2023 20:55
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.

None yet

4 participants