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

Deprecate 'R' in C++ setter nomenclature in favor of 'D' #1433

Merged
merged 6 commits into from
Feb 11, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 9, 2023

Changes proposed in this pull request

While Cantera's Python nomenclature uses TD/TDX/TDY and DP/DPX/DPY, other API's use the older (?) nomenclature TR/TRX/TRY and RP/RPX/RPY. This PR seeks to implement a uniform nomenclature.

In addition, a new

    CANTERA_CAPI int thermo_set_TD(int n, double* vals);

is added to ct.h, which is needed for work in #1182.

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

codecov bot commented Feb 9, 2023

Codecov Report

Merging #1433 (2b5eb74) into main (808023c) will decrease coverage by 0.04%.
The diff coverage is 43.24%.

@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
- Coverage   70.96%   70.92%   -0.04%     
==========================================
  Files         369      369              
  Lines       55201    55229      +28     
  Branches    18176    18194      +18     
==========================================
  Hits        39173    39173              
- Misses      13567    13595      +28     
  Partials     2461     2461              
Impacted Files Coverage Δ
include/cantera/thermo/PDSS.h 100.00% <ø> (ø)
include/cantera/thermo/PDSS_ConstVol.h 100.00% <ø> (ø)
include/cantera/thermo/PDSS_Water.h 100.00% <ø> (ø)
include/cantera/thermo/Phase.h 77.77% <ø> (ø)
include/cantera/thermo/ThermoPhase.h 44.58% <0.00%> (ø)
include/cantera/thermo/WaterPropsIAPWS.h 100.00% <ø> (ø)
interfaces/dotnet/Cantera/src/Enums.cs 91.66% <ø> (ø)
src/clib/ct.cpp 13.47% <0.00%> (-0.18%) ⬇️
src/thermo/MixtureFugacityTP.cpp 41.11% <0.00%> (ø)
src/thermo/PDSS.cpp 39.41% <0.00%> (-0.89%) ⬇️
... and 16 more

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

@ischoegl ischoegl marked this pull request as ready for review February 9, 2023 17:52
@ischoegl ischoegl requested a review from a team February 9, 2023 17:53
@ischoegl ischoegl mentioned this pull request Feb 9, 2023
5 tasks
@speth
Copy link
Member

speth commented Feb 9, 2023

I appreciate the attempt to improve the consistency of the different interfaces.

I'm wondering about the usefulness of this whole family of functions, though -- basically, setState_DP, setState_DPX, and setState_DPY, with the latter two having three variants each depending on how the mass/mole fractions are specified. I note the following:

  • Besides setState_TPX and setState_TPY, these are the only "full-state" setters that we provide in C++.
  • The only thermo model that implements setState_DP is IdealGasPhase.
  • The setState_PV method is essentially equivalent, but this method is implemented only for PureFluidPhase.
  • I'm having trouble coming up with the physical situation where this is the property pair that you know and that the thing that needs to be found is the temperature. It seems like a pretty unusual situation.
  • This was all added by @bryanwweber some time ago, so maybe he remembers what these functions were for?

Unless there's some interesting use case, I think I'd support deprecating all of the "full-state" versions at least and combining setState_DP and setState_PV so only one of them needs to be implemented in any specific thermo model.

@bryanwweber
Copy link
Member

bryanwweber commented Feb 9, 2023

I'm not totally sure which functions you're referring to @speth. I will say that, with respect to the PV property pair, these are extremely common in thermodynamic systems from intro textbooks. Or, possibly, in a cycle where the volume or pressure are held constant through a process, so you don't know the temperature at the other end (for example, "the mixture is heated at constant pressure until the volume has tripled").

Unless there's some interesting use case, I think I'd support deprecating all of the "full-state" versions at least and combining setState_DP and setState_PV so only one of them needs to be implemented in any specific thermo model.

I think removing the full-state versions would be fine. I think as long as both DP and PV are available (even if they're dependent/interrelated) it would also be fine.

@ischoegl
Copy link
Member Author

ischoegl commented Feb 9, 2023

Looks like I opened a can of worms here ... this was supposed to be simple! 🤣

My main incentive was to make the nomenclature consistent, but I do see that there's a lot of boilerplate code. At the same time, the setState_TPX/setState_TPY are used extensively in examples and the test suite, so I'm not necessarily in support of removing those just for the sake of avoiding a handful of service functions (which can likely be inlined/header-only).

I tend to agree that keeping both setState_PV and setState_DP make sense; @bryanwweber mentions an example that is indeed often used in thermo (another one is an IC engine where you know pressures and specific volume from experimental data). Regarding DP vs PV, this probably depends on a user perspective. Specific volumes are certainly common when viewed from a thermo perspective, but in other contexts, I'd say that density is probably more natural (compressible flows?). As an aside, the same argument goes for TD and TV ... in both cases, context is key.

@bryanwweber
Copy link
Member

so I'm not necessarily in support of removing those just for the sake of avoiding a handful of service functions

I think @speth was suggesting to remove PDY and PDX, as those aren't as likely to be used.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Just a couple comments for food for thought

interfaces/matlab/toolbox/@ThermoPhase/setState_DP.m Outdated Show resolved Hide resolved
src/thermo/PDSS.cpp Show resolved Hide resolved
src/thermo/PDSS_ConstVol.cpp Show resolved Hide resolved
@ischoegl ischoegl force-pushed the deprecate-R-in-setters branch 2 times, most recently from c9c5f47 to b51a06c Compare February 10, 2023 03:03
@ischoegl
Copy link
Member Author

@speth and @bryanwweber ... I deprecated all full state setters using R (i.e. TRX/TRY and RPX/RPY) but retained the TD and DP versions.

@ischoegl ischoegl requested a review from a team February 10, 2023 23:54
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.

Looks good to me!

I wasn't originally advocating for the removal of the TPX/TPY setters or the TRX/TRY setters, but I'm quite happy to see the latter set go.

@ischoegl ischoegl merged commit ba76a7b into Cantera:main Feb 11, 2023
@ischoegl ischoegl deleted the deprecate-R-in-setters branch February 13, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants