-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Fix issues with "volume" and compressibility of SurfPhase #1350
Conversation
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.
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 |
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. |
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. |
include/cantera/thermo/SurfPhase.h
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the units in these docstrings. Aren't they inverted from those of concentrations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I was just correcting the fact that it's m^2 for surfaces, but this is clearly inverted as well.
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 Would this be a tenable approach? Any variable whose name specifically references volume or v would use Also, would |
@decaluwe … thanks for your input on this. I think |
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. |
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...) 🤦 |
Yes, setting the
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. |
Perfect - I’m happy with that outcome! |
5c5791e
to
cc900a3
Compare
Codecov Report
@@ 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
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@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 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". |
3080aa2
to
9ea4675
Compare
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
9ea4675
to
0847538
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @speth - this looks good to me!
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. |
Changes proposed in this pull request
SurfPhase
to be zero.SurfPhase
to be per unit area (and per unit length forEdgePhase
.SurfPhase
incompressible, which disables setting the density to a value inconsistent with the site density.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
scons build
&scons test
) and unit tests address code coverage