Add PureFluid properties

[bryanwweber]

The PureFluid classes allow getting/setting a larger number of properties than were previously implemented. This PR adds those missing functions, leaving them unimplemented in the general ThermoPhase, and implementing them only in the PureFluid.

There is a small issue with the algorithm to set the properties, in that the final set value for a property depends on the order in which the properties were previously set. This results in the "expected" failure in the Python tests. I'm not really sure how to fix this; I tried tightening the tolerances in the Sub.cpp file, with no effect. Nonetheless, the error is quite small, but still exceeds the defaults set up in the tester.

[speth]

I feel like the phrasing that this method "must be implemented in a derived class" is misleading.

[bryanwweber]

It is there to indicate that this method will throw a NotImplemented error if it is used without implementing it. That phrasing is present on a bunch of the similar functions. Is there an alternative phrasing you would prefer?

[decaluwe]

Hmm, I don’t know specifically what Ray finds misleading, but something along the lines of “Implementation must occur at the level of the derived class; absent this, the method throws a “NotImplemented” error” might be more detailed?

On Jan 20, 2016, at 1:36 PM, Bryan W. Weber notifications@github.com wrote:

In include/cantera/thermo/ThermoPhase.h #315 (comment):

@@ -975,6 +974,118 @@ class ThermoPhase : public Phase
*/
virtual void setState_SV(doublereal s, doublereal v, doublereal tol = 1.e-4);

• //! Set the specific entropy (J/kg/K) and temperature (K).
• /*!

• \* This function fixes the internal state of the phase so that the specific
  

• \* entropy and temperature have the value of the input parameters.
  

• \* It must be implemented in a derived class.
  

  It is there to indicate that this method will throw a NotImplemented error if it is used without implementing it. That phrasing is present on a bunch of the similar functions. Is there an alternative phrasing you would prefer?

—
Reply to this email directly or view it on GitHub https://github.com/Cantera/cantera/pull/315/files#r50314799.

[speth]

I guess I'm looking for a phrasing that distinguishes between methods like ThermoPhase::pressure, which if left unimplemented causes a large number of other methods to not work as well, and this, which just means that this one method has no implementation in the base class.

[speth]

I think the tolerances in the Substance class are all hard coded; there's no argument that can be used to tighten them.

The default tolerance of assertNear is arbitrary, too, and I don't think there's anything wrong with specifying a value that corresponds to a known tolerance in the state setting algorithm.

[bryanwweber]

Yes, they are hardcoded into Sub.cpp (or Sub.h or perhaps PureFluid.cpp, I forget), but even changing them there had no impact on the value of the final state. Changing the order of state setting did have an impact on the final value, which I guess is my bigger issue here. Is that something that should bother me very much, or should I mark it down as an inherent issue with the method?

[speth]

Setting TolRel = 1e-8 in Sub.cpp caused the test to pass for me. I'm not sure how many of the other tolerances in that file are applicable.

Within the tolerance bounds, the precise stopping point of an iterative solver is going to depend on the initial guess. If you wanted to make the solution independent of the previous state of the object, you'd need to always start from the same state. That might be worth doing, or it might introduce other problems, for instance when trying to calculate differences between nearby states. But I think that's independent of making these setters available in the ThermoPhase interface.

[bryanwweber]

OK, so I must have been going crazy, because I would swear that didn't work last time I tried it. Nonetheless, it works now and I rebased all the commits into a sensible order. I changed the comment in ThermoPhase.h to say "This base class function will print an error if not overwritten." instead of what I had before.

It looks like some of the C++ tests are failing, likely due to changing the tolerances. I'll see what I can do about that.

They were failing because I changed one of the tolerances. Its fixed now.

[speth]

The correct term here is overridden, not overwritten, no matter how many times latter has been used by a certain developer.

[bryanwweber]

Whoops... That would be (mostly) me... Fixed now

[bryanwweber]

Should I rebase this onto the new HEAD commit efd6d23 before it gets merged? Should we have a "policy" or something about whether merges are OK or we prefer rebases? I prefer a linear commit history without merge commits (i.e., fast-forward merges only), but its not a very strong preference.

[speth]

I definitely prefer the linear commit history. My method for "merging" pull requests is basically to manually fetch the changes, rebase onto master, make any fixups I want, and then push. The only minor downside to this approach is that this doesn't mark the PR as "merged" on Github, and you can only mark it "closed".

[speth]

Which is to say, you don't need to rebase the PR unless you're just going to push it to master yourself.

[bryanwweber]

I think if the PR gets rebased before merging, and the rebase gets pushed to the branch, then it gets marked as "merged". At least, that's what happened to #314. I don't usually like to merge bigger changes like this to master myself, because I'm not (yet) sure when they meet the standards, and IMO its better to make back-and-forth and small changes in a branch that I can force push to than in the main master branch. So I'll rebase this onto the new master then, with the nitpick you suggested.

Re spaces after commas, I would prefer spaces after all of the commas, even if PEP8 doesn't say anything - it just looks more natural to me. I suppose another reason is that I like to have constant linting running, which doesn't like no spaces after commas no matter the variable length. Nonetheless, since you asked, I'll change them back.

Also, regarding my switch from doublereal to double, I thought it would be better to be consistent within all of those functions. Since I was defining a bunch of them as new, the old ones would be doublereal and the new ones would be double, and I didn't think that would be any better than the (possible) code churn with the conversion.

At the very least, having the typedef in place at all is already very confusing to a new person like me, and I would advocate a switch to one or the other entirely, regardless of future plans (and preferably double, obviously 😃). Why is it there in the first place? From Google research, I guess it has something to do with the C API and Fortran data types?

[speth]

Yes, if you rebase first, then Github will recognize that the PR is being "merged". I appreciate the opportunity to iterate on these changesets before pushing to master. Generally, though, there are going to be (and have been) PRs that aren't rebased by their authors first, and therefore appear as "closed", so the PR status itself isn't really meaningful.

My preference for being able to omit the space after a comma isn't critical, but there's a lot of code in Cantera that is written this way, and I'm a little ambivalent about applying a minor style change in a piecemeal fashion. Maybe you can turn this rule off in your linter?

I'm not quite sure what the original thought behind doublereal was. I have trouble believing that there is any system where the C double type isn't the same as the Fortran real*8. I would say that changing doublereal to double would be best saved for a point where some other widespread changes are going to be made, since it will create a point in the code where it's difficult to merge or rebase across since the change will touch so many lines.