Fix issues with "volume" and compressibility of SurfPhase #1350

speth · 2022-08-02T03:18:20Z

Changes proposed in this pull request

Define the molar volume of a SurfPhase to be zero.
Define concentration and density-like quantities of SurfPhase to be per unit area (and per unit length for EdgePhase.
Make SurfPhase incompressible, which disables setting the density to a value inconsistent with the site density.
Modify conversion between mole fractions and coverages to avoid needing to use the density
Fix handling of variable species "sizes" / multi-site species, which was incorrect and not subject to any tests
Fix setting surface state using mole fractions, which did not actually preserve that sum(concentrations) == coverages. This change is responsible for the changes to the blessed output of the F90 demo.
Fix a couple of issues with problems generating detailed error messages
Fix a couple of tangentially-related issues with the F90 "demo" program

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

Closes #1312

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

src/thermo/SurfPhase.cpp

ischoegl · 2022-08-03T21:16:16Z

Per @bryanwweber's request, I am putting a line comment from #1350 (comment) into the main thread:

It is true that [setting density] zero wouldn't be consistent with the original intent stated at the top, i.e.

Define the "volume" of a SurfPhase to be zero. This implies that the density must be inf.

However: as the concept of a volume evaluated for a 2D geometry is fundamentally flawed I am starting to wonder whether this shouldn't be better handled as an exception?

speth · 2022-08-05T01:48:30Z

However: as the concept of a volume evaluated for a 2D geometry is fundamentally flawed I am starting to wonder whether this shouldn't be better handled as an exception?

By "as an exception", you mean by having density(), molarDensity() and molarVolume() all return zero? I guess that would save the trouble of having to handle inf in YAML input/output, at least.

ischoegl · 2022-08-05T02:45:23Z

By "as an exception", you mean by having density(), molarDensity() and molarVolume() all return zero? I guess that would save the trouble of having to handle inf in YAML input/output, at least.

I was thinking about returning zero while issuing a warning (potentially to be turned into an error after Cantera 3.0).

speth · 2022-08-05T02:59:45Z

I was thinking about returning zero while issuing a warning (potentially to be turned into an error after Cantera 3.0).

I think that issuing warnings or errors about accessing these properties would be more of a headache than it's worth. I'm pretty sure there are a lot of places that assume that density is a useful part of a phase's state.

ischoegl · 2022-08-05T03:18:53Z

I'm pretty sure there are a lot of places that assume that density is a useful part of a phase's state.

I would certainly agree that it's a useful quantity for 3D phases, but for 2D it becomes quite murky, hence my suggestion for a warning / exception.

decaluwe · 2022-08-05T12:58:13Z

include/cantera/thermo/SurfPhase.h

@@ -189,8 +210,8 @@ class SurfPhase : public ThermoPhase
     *
     * @param k Optional parameter indicating the species. The default
     *          is to assume this refers to species 0.
-     * @return
-     *   Returns the standard Concentration in units of m3 kmol-1.
+     * @return the standard concentration in units of m^2/kmol for surface phases or


I don't understand the units in these docstrings. Aren't they inverted from those of concentrations?

Good catch. I was just correcting the fact that it's m^2 for surfaces, but this is clearly inverted as well.

decaluwe · 2022-08-05T13:10:42Z

Been following this convo and pondering. To me, the concept of density still applies, but its units just change with the dimensionality of the phase. So density would have units of kg/m^3 for a 3-D phase, and kg/m^2 for a 2-D phase. I don't really know how useful this quantity would be for a surface phase, but that would match what we do with concentrations, no?

Would this be a tenable approach? Any variable whose name specifically references volume or v would use volume = 0, whereas anything that does not would adjust the implicit dimensionality? So concentrations and densities would be per m^2 (or per m for edge phases)? I looked through the files and noticed that many calls related to concentration and setConcentration were removed--are we trying to move away from/remove those functions?

Also, would v = 0 imply that h = u for a surface phase? I guess I should go and look at what our current approach is, here.

ischoegl · 2022-08-05T13:19:09Z

@decaluwe … thanks for your input on this. I think density being redefined based on 2D units would make sense. The only (friendly) amendment I have is that an attempt to access any variable whose name specifically references volume or v should also raise a warning (or error after Cantera 3.0) … that way we can hopefully avoid the introduction of inf.

speth · 2022-08-06T00:18:35Z

I looked through the files and noticed that many calls related to concentration and setConcentration were removed--are we trying to move away from/remove those functions?

This is related to enforcing the idea that this is an "incompressible" phase, and that the site density is fixed (unless explicitly changed). Setting the vector of concentrations is overconstrained, since the sum of the concentrations must equal the site density.

decaluwe · 2022-08-07T01:41:46Z

Thanks, @speth --that makes sense. One related question - can the user still change the site density via the python interface (i.e. "on the fly")? I could see that remaining a useful feature, in terms of having that be a "fittable" parameter. But I would be in favor of requiring any altering of that parameter to require a certain amount of "intentionality," i.e. they should not be able to do it unwittingly.

(ps. sorry about the accidental close + reopen. Was trying to close a draft comment, and clicked "close with comment" before I thought it all the way through...) 🤦

speth · 2022-08-07T02:35:30Z

ne related question - can the user still change the site density via the python interface (i.e. "on the fly")? I could see that remaining a useful feature, in terms of having that be a "fittable" parameter.

Yes, setting the site_density property in Python (and the equivalent in C++) is still possible.

Would this be a tenable approach? Any variable whose name specifically references volume or v would use volume = 0, whereas anything that does not would adjust the implicit dimensionality? So concentrations and densities would be per m^2 (or per m for edge phases)?

Having thought about this a bit more, I think I agree that this works. Basically, it's saying that "volume = 1/density" is only meaningful for bulk (3D) phases, which sort of makes sense. This sidesteps the need to have anything return "inf", so I don't think it's necessary for any methods to raise warnings or exceptions.

ischoegl · 2022-08-07T11:35:29Z

Having thought about this a bit more, I think I agree that this works. Basically, it's saying that "volume = 1/density" is only meaningful for bulk (3D) phases, which sort of makes sense. This sidesteps the need to have anything return "inf" […]

Perfect - I’m happy with that outcome!

codecov · 2022-08-07T18:57:59Z

Codecov Report

Merging #1350 (d317f12) into main (ee1ad0b) will increase coverage by 0.00%.
The diff coverage is 77.77%.

@@           Coverage Diff           @@
##             main    #1350   +/-   ##
=======================================
  Coverage   68.15%   68.16%           
=======================================
  Files         327      327           
  Lines       42659    42690   +31     
  Branches    17180    17189    +9     
=======================================
+ Hits        29075    29098   +23     
- Misses      11291    11298    +7     
- Partials     2293     2294    +1

Impacted Files	Coverage Δ
include/cantera/thermo/LatticeSolidPhase.h	`36.66% <0.00%> (ø)`
include/cantera/thermo/Phase.h	`83.67% <ø> (ø)`
src/fortran/fct.cpp	`19.96% <ø> (ø)`
include/cantera/base/AnyMap.h	`79.41% <20.00%> (-4.97%)`	⬇️
include/cantera/thermo/SurfPhase.h	`83.33% <57.14%> (-16.67%)`	⬇️
src/thermo/SurfPhase.cpp	`89.39% <92.00%> (-0.17%)`	⬇️
src/base/Units.cpp	`83.98% <100.00%> (+0.08%)`	⬆️
src/kinetics/solveSP.cpp	`67.16% <100.00%> (+0.33%)`	⬆️
include/cantera/cython/thermo_utils.h	`95.45% <0.00%> (-4.55%)`	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

bryanwweber · 2022-08-07T19:03:05Z

Having thought about this a bit more, I think I agree that this works.

@speth As we've reached a consensus, can you document/summarize this outcome in code comments/docstrings, since it may not be obvious how or why we ended up here? And also link to this PR somewhere in the code?

speth · 2022-08-07T19:52:49Z

Having thought about this a bit more, I think I agree that this works.

@speth As we've reached a consensus, can you document/summarize this outcome in code comments/docstrings, since it may not be obvious how or why we ended up here? And also link to this PR somewhere in the code?

If you read the changes, I believe that I have documented the new(ish) behavior, with additions to the docstring for the Phase class and in the newly overloaded members of the SurfPhase class. The former is just documenting what was already the case, and I think the docstrings for the latter are brief but clear. I don't think it's necessary to link to this PR anywhere -- as far as I can tell, that's never something we've done in the past.

P.S. I would have thought I might get a little bit of space to work on this without everyone piling on, given both that it is marked "Draft" and I did not check the box for "The pull request is ready for review".

Defines molar volume to be zero. Density remains defined as being per unit area or length, depending on the dimensionality of the phase. Also fixes setting surface composition using mass/mole fractions so that the sum of concentrations always equals the site density. Partially resolves Cantera#1312

Partially resolves Cantera#1312

ischoegl

Thanks, @speth - this looks good to me!

bryanwweber · 2022-08-08T22:38:11Z

I don't think it's necessary to link to this PR anywhere -- as far as I can tell, that's never something we've done in the past.

To my naïve eye, this looks to have generated a fair amount of in-the-weeds thermo/physical chemistry discussion. As the reasoning for why we're making this choice of behavior may not be clear in 3 months or 5 years or..., especially to new developers, it seems useful to me to provide a link to the nitty gritty of the discussion rather than adding all of that into a comment or something, or eliding it and assuming the result is obvious. YMMV 🤷‍♂️ but I won't hold up the PR over this.

ischoegl · 2022-08-09T00:29:39Z

Personally, I believe that updated docstrings, together with links in commits e6f3707 and 656dfe2, are adequate.

speth added the Thermo label Aug 2, 2022

ischoegl reviewed Aug 2, 2022

View reviewed changes

src/thermo/SurfPhase.cpp Outdated Show resolved Hide resolved

decaluwe reviewed Aug 5, 2022

View reviewed changes

decaluwe closed this Aug 7, 2022

decaluwe reopened this Aug 7, 2022

Avoid treating simple strings as format strings

2d6c15a

speth force-pushed the fix-surfphase-volume branch 2 times, most recently from 5c5791e to cc900a3 Compare August 7, 2022 18:20

speth force-pushed the fix-surfphase-volume branch from 3080aa2 to 9ea4675 Compare August 7, 2022 20:41

speth added 7 commits August 7, 2022 17:00

Fix duplication of input context in InputFileError

5994e30

Make SurfPhase incompressible

656dfe2

Partially resolves Cantera#1312

Fix getCoverages for multi-site species

c788042

[Fortran] Fix compilation error from getReactionType

16e4543

[Fortran] Fix compilation warnings from Clang

beac37f

Fix incorrect name/label in F90 sample

0847538

speth force-pushed the fix-surfphase-volume branch from 9ea4675 to 0847538 Compare August 7, 2022 21:00

Restore test coverage of setConcentrations

d317f12

speth marked this pull request as ready for review August 8, 2022 14:37

ischoegl approved these changes Aug 8, 2022

View reviewed changes

ischoegl merged commit 755a800 into Cantera:main Aug 9, 2022

speth deleted the fix-surfphase-volume branch August 9, 2022 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with "volume" and compressibility of SurfPhase #1350

Fix issues with "volume" and compressibility of SurfPhase #1350

speth commented Aug 2, 2022 •

edited

Loading

ischoegl commented Aug 3, 2022

speth commented Aug 5, 2022

ischoegl commented Aug 5, 2022

speth commented Aug 5, 2022

ischoegl commented Aug 5, 2022

decaluwe Aug 5, 2022

speth Aug 6, 2022

decaluwe commented Aug 5, 2022

ischoegl commented Aug 5, 2022 •

edited

Loading

speth commented Aug 6, 2022

decaluwe commented Aug 7, 2022 •

edited

Loading

speth commented Aug 7, 2022

ischoegl commented Aug 7, 2022 •

edited

Loading

codecov bot commented Aug 7, 2022 •

edited

Loading

bryanwweber commented Aug 7, 2022

speth commented Aug 7, 2022

ischoegl left a comment

bryanwweber commented Aug 8, 2022

ischoegl commented Aug 9, 2022

Fix issues with "volume" and compressibility of SurfPhase #1350

Fix issues with "volume" and compressibility of SurfPhase #1350

Conversation

speth commented Aug 2, 2022 • edited Loading

ischoegl commented Aug 3, 2022

speth commented Aug 5, 2022

ischoegl commented Aug 5, 2022

speth commented Aug 5, 2022

ischoegl commented Aug 5, 2022

decaluwe Aug 5, 2022

Choose a reason for hiding this comment

speth Aug 6, 2022

Choose a reason for hiding this comment

decaluwe commented Aug 5, 2022

ischoegl commented Aug 5, 2022 • edited Loading

speth commented Aug 6, 2022

decaluwe commented Aug 7, 2022 • edited Loading

speth commented Aug 7, 2022

ischoegl commented Aug 7, 2022 • edited Loading

codecov bot commented Aug 7, 2022 • edited Loading

Codecov Report

bryanwweber commented Aug 7, 2022

speth commented Aug 7, 2022

ischoegl left a comment

Choose a reason for hiding this comment

bryanwweber commented Aug 8, 2022

ischoegl commented Aug 9, 2022

speth commented Aug 2, 2022 •

edited

Loading

ischoegl commented Aug 5, 2022 •

edited

Loading

decaluwe commented Aug 7, 2022 •

edited

Loading

ischoegl commented Aug 7, 2022 •

edited

Loading

codecov bot commented Aug 7, 2022 •

edited

Loading