[Thermo] Adds phaseOfMatter function #722
Conversation
Codecov Report
@@ Coverage Diff @@
## master #722 +/- ##
==========================================
+ Coverage 71.4% 71.41% +0.01%
==========================================
Files 372 372
Lines 43863 43904 +41
==========================================
+ Hits 31320 31355 +35
- Misses 12543 12549 +6
Continue to review full report at Codecov.
|
bryanwweber
force-pushed
the
add-get-phase
branch
from
0dc02e0
to
5fe69ac
Compare
bryanwweber
force-pushed
the
add-get-phase
branch
from
5fe69ac
to
4b04c33
Compare
|
@bryanwweber ... sure. Before leaving more detailed feedback, I have a philosophical question. I am very much in favor of using small utility functions like My main concern of this PR is that it appears to be geared towards a very special case, rather than being applicable to any thermo phase. Would it make sense to implement |
There was a problem hiding this comment.
I like the idea of at least making room to implement this for most ThermoPhase objects, which would suggest using names that aren't particular to the pure fluid equation of state model. @ischoegl's suggestions of gas, liquid, and gas-liquid-mix seem reasonable to me.
I wouldn't necessarily suggest adding implementations for all of the other ThermoPhase types here. I suspect that we'll find some cases where there's not a clear choice of what to return. The one that comes to mind immediately is IdealSolidSolnPhase, which, despite the name, is equally applicable to solids and liquids, and has no information on which kind of phase it is currently implementing.
I think it would make sense to add this information to the output of PureFluidPhase::report. If we wanted to add it to the report for all phases, we could change the default implementation to just return unspecified.
I think this will make sense eventually, and like @speth I think the names you've suggested are very good. However, at this point, I think it is better to have only one phase where this works, and we know that it works. Since, as @speth notes, for many phases it isn't clear what should be returned, I don't want to dig into adding that kind of functionality here, I think that could be done as a general cleanup/improvement of each phase individually. |
I can certainly see this perspective. Is there a good way to hit a middle ground? I.e. add the information where it is evident (i.e. we know that it works), and defer the tricky ones to #652 (which btw miraculously disappeared). |
We just created a new repository so that we could have a separate issue tracker for some of these bigger-picture enhancements to Cantera, and I moved #652 there (now Cantera/enhancements#6). If you use the old URL (https://github.com/Cantera/cantera/issues/652) it will redirect, but unfortunately it looks like the automatic links just using the issue number are now broken. |
Ah. I noticed this new repo, and start to see what it's for. Makes sense. It'll be interesting to see how this evolves. |
|
@ischoegl the idea is that this would be a place to document longer-term development goals, projects, etc, separate from bugs and quicker fixes in the main repo issues. So that people know what others are working on, what is coming down the pike, etc., and also a place to make those types of requests. Hopefully, this will eventually be a place to host a clearer development roadmap. |
|
@decaluwe ... thanks for the info. It may be good to have a template that illustrates the mechanism for how enhancements can get merged to master. I.e. is this just a fork or something entirely separate? This discussion is admittedly off-topic, but my main point was that some incremental improvements in line with (former) #652 could be applied whenever appropriate. |
|
@ischoegl I'm not sure I understand what you mean by "a template that illustrates the mechanism for how enhancements can get merged to master" -- that process will still be a PR to |
|
Here are the names I came up with:
The last one is 20 characters wide, whereas the current fields in the Not so much for the ones that are longer: |
|
This is a bit more dramatic change, but what about giving up on right aligning the numbers, given that none of them are comparable or have a similar number of digits to the right of the decimal point? |
bryanwweber
force-pushed
the
add-get-phase
branch
from
4b04c33
to
c47c11c
Compare
bryanwweber
changed the title
bryanwweber
force-pushed
the
add-get-phase
branch
2 times, most recently
from
2cb6b25
to
bf949cc
Compare
|
@speth @ischoegl I made a bunch of changes in bf949cc to make the strings used in the report methods composable. I'm not sure it improved readability and would appreciate your opinions. One of the reasons I wanted to use variables to format the strings is in anticipation of new and/or different properties that might be reported for different phase types. |
Hmm, that's interesting, but I agree that this creates some significant readability challenges. I find the doubly-substituted format strings to be particularly mind-bending. While making the string two_property = "{:>18} {:12.5g} {:12.5g} {}\n";
...
format_to(b, two_property, "enthalpy", enthalpy_mass(), enthalpy_mole(), "J"); |
|
@speth OK, that makes sense. One of the things I'd like to do is keep the width of the first field as a variable, since I anticipate that's one of the things most likely to change if we add new properties, if a property's name is too long, we want to support longer species names, etc. That's actually how 0ce4f1b is implemented, and combined with your suggestion, I think it could still be pretty readable. Another thing I forgot to ask... in 4dba1d1 I switched |
That looks fine to me. Limited use of the variable field width syntax seems fine. It's really just the case of using One minor issue I noticed is that this leaves trailing whitespace on the line which reads:
I'm 👍 on this change. I had been hoping that @rwest would submit a fix for this himself. Unsurprisingly, there are some test failures associated with this change that need to be updated. |
|
So we've got the old output as vs the proposed version What about a compromise, where an additional newline separates the state and other currently not shown information is added, i.e. |
I always wondered why my thermo book did this. Fortunately, it should now be much easier to be explicit here, will happily update! @ischoegl What about putting the extra information behind a flag that's default @speth I'll look into those EOL spaces on the header line. And yes, the regression tests... well, I didn't get to them yet 😄 |
ugh, yes (which has been aggravating in teaching). I'd still suggest to stick with Kee's notation, as everything else in Cantera uses |
|
Oh, I wasn’t suggesting we switch to IUPAC (just perhaps also label it “mass fraction”) |
|
@bryanwweber ... the formatting was just a suggestion. I have already added |
|
Alright. Going back to the original issue (with an insufficient column width), why not just expanding it to 15 characters? Overall, this still stays below the 80 character line length recommendation, plus allows to add PS: the only phase of matter that won't align nicely with 15 characters is thus |
|
Here's where I've landed now:
|
|
@bryanwweber ... I think any solution will work, but still believe that the old right-adjusted way is quicker to parse visually (also, it is consistent with the specific properties block right underneath) Regarding chem. potential I agree that lowercase is more consistent. For trailing white space, right adjusting PS: any opinions on replacing |
bryanwweber
force-pushed
the
add-get-phase
branch
from
bf949cc
to
c9b308f
Compare
src/thermo/PureFluidPhase.cpp
Outdated
| format_to(b, one_property, "density", name_width, density(), "kg/m^3"); | ||
| format_to(b, one_property, "mean mol. weight", name_width, meanMolecularWeight(), "amu"); | ||
| format_to(b, "{:>{}} {:15.6g}\n", "vapor fraction", name_width, vaporFraction()); | ||
| format_to(b, "{:>{}} {:>15}\n", "phase", name_width, phaseOfMatter()); |
There was a problem hiding this comment.
Should this be "phase of matter"?
bryanwweber
force-pushed
the
add-get-phase
branch
from
c9b308f
to
0730d1d
Compare
|
Here's the format I finished with: I've now updated all the regression tests, so hopefully everything passes this time! |
There was a problem hiding this comment.
I'm satisfied with the revised report() format. I wish the code to generate it were a little less cumbersome, but that seems to be how it goes with string formatting code most of the time, and we can always come back to this later.
I think if we're adding a phase of matter field to the report, implementations of that method for at least the easy and commonly-used phases need to be added at this time. It's okay if there are a few complicated cases that are deferred until later.
bryanwweber
force-pushed
the
add-get-phase
branch
from
0730d1d
to
e6df25d
Compare
|
@speth I was only able to determine definite phases for |
phaseOfMatter returns the mechanical phase (liquid, gas, etc.) of the matter in the Phase. The default value is 'unspecified' for phases that don't contain enough information to determine the mechanical phase.
The Phase report now includes the mechanical phase of the matter in the Phase. The report is reformatted to use field widths consistently and realign some of the data to better fit the mechanical phase type.
The columns for mole and mass fraction are swapped in the output of report. This makes the two property tables consistent with mass listed first and then moles.
Make the strings in the report method composable. Enables setting string parameters by variables at the top of the function.
Python 3.8 now warns about misuse of the is comparison operator.
These regression tests include the output of ThermoPhase::report in the test comparison.
bryanwweber
force-pushed
the
add-get-phase
branch
from
e6df25d
to
2bf1a7d
Compare
Changes proposed in this pull request:
phaseOfMatter()returns the mechanical phase (liquid, vapor, etc.) of the matter in the Phase.Not sure if this will be more generally useful. I was using it to help debug #721, but added it here to the
PureFluidPhaseinstead ofWaterSSTPbecause I don't want to touchWaterSSTPwith a 39.5-foot pole. I could add a similar function toWaterSSTPthough, if desired.