Some Matlab functions to set the state of PureFluids are broken

[bryanwweber]

Matlab R2014a, Cantera 2.2.0, Windows 7 (but probably not OS-specific). The three title functions either throw errors or don't work as expected.

>> w = Water();
>> w

  water:

       temperature             300  K
          pressure          101325  Pa
           density         996.633  kg/m^3
  mean mol. weight          18.016  amu
    vapor fraction               0

                          1 kg            1 kmol
                       -----------      ------------
          enthalpy    -1.58581e+007      -2.857e+008     J
   internal energy    -1.58582e+007      -2.857e+008     J
           entropy         3913.17        7.05e+004     J/K
    Gibbs function    -1.70321e+007      -3.068e+008     J
 heat capacity c_p         4180.79       7.532e+004     J/K
 heat capacity c_v            4131       7.442e+004     J/K

>> setState_Tsat(w, 400)
Error using ctmethods
wrong size

Error in thermo_set (line 5)
    i = ctmethods(20,n,-job,a);

Error in ThermoPhase/setState_Tsat (line 11)
thermo_set(tp.tp_id, 25, tx);

>> setState_satLiquid(w) % No change in state
>> w

  water:

       temperature             300  K
          pressure          101325  Pa
           density         996.633  kg/m^3
  mean mol. weight          18.016  amu
    vapor fraction               0

                          1 kg            1 kmol
                       -----------      ------------
          enthalpy    -1.58581e+007      -2.857e+008     J
   internal energy    -1.58582e+007      -2.857e+008     J
           entropy         3913.17        7.05e+004     J/K
    Gibbs function    -1.70321e+007      -3.068e+008     J
 heat capacity c_p         4180.79       7.532e+004     J/K
 heat capacity c_v            4131       7.442e+004     J/K

>> setState_satVapor(w)
Error using ctmethods
unknown attribute.

Error in thermo_set (line 5)
    i = ctmethods(20,n,-job,a);

Error in ThermoPhase/setState_satVapor (line 9)
thermo_set(tp.tp_id, 3, 0);

[speth]

• Also affects setState_Psat
• Confirmed that the results are the same under Linux.
• Just for kicks, I checked on an old install of Cantera "1.8", and the behavior is the same. My guess is that these functions have never worked.

[bryanwweber]

Thanks Ray! The code in setState_satLiquid is thermo_set(tp.tp_id, 2, 0);, which actually ends up calling the th_setElectricPotential function (probably why it doesn't do anything). A fix for the satLiquid and satVapor functions shouldn't be too hard, they can just call the existing set function with the appropriate value for Vapor or Liquid fraction, which does work. The Tsat and Psat functions look like they just need an additional argument called x in src/matlab/ct.h. I suppose that means it should be the quality/vapor fraction. I'll see if I can gin up a fix this weekend.

[speth]

Looking at set.m, I realize that setState_Psat and setState_Tsat aren't broken, it's just that the documentation is wrong. The correct calls would be:

setState_Psat(w, [pressure, vapor_fraction])
setState_Tsat(w, [temperature, vapor_fraction])

which is consistent with how all of the other setState methods work as well.

[bryanwweber]

Is there a reason why a vector is used there instead of two separate arguments? The setState_Psat function in the library uses two arguments, instead of a vector: https://github.com/Cantera/cantera/blob/master/src/thermo/PureFluidPhase.cpp#L336

To me, two arguments makes much more sense and I'd prefer to switch the Matlab interface to use two arguments. It would be a breaking change, is the only thing...

[decaluwe]

Can you configure to accept both and print out a warning that it will soon be deprecated?

Steven

On Oct 23, 2015, at 1:49 PM, Bryan W. Weber notifications@github.com wrote:

Is there a reason why a vector is used there instead of two separate arguments? The setState_Psat function in the library uses two arguments, instead of a vector: https://github.com/Cantera/cantera/blob/master/src/thermo/PureFluidPhase.cpp#L336 https://github.com/Cantera/cantera/blob/master/src/thermo/PureFluidPhase.cpp#L336
To me, two arguments makes much more sense and I'd prefer to switch the Matlab interface to use two arguments. It would be a breaking change, is the only thing...

—
Reply to this email directly or view it on GitHub #299 (comment).

[bryanwweber]

Actually, I think I figured out that it does have to be a vector, due to the way the thermomethods.cpp MEX stuff expects its input. Thus, changing it to accept two inputs would require a change in the underlying C++ code, which doesn't seem worth it for this small inconsistency. I'll just change the documentation for the setState_Psat and setState_Tsat functions, and not change the interface.

[speth]

I think it should remain the way it is. Changing it would make it inconsistent with the other Matlab functions like setState_HP, and I think consistency within a language interface is more important than consistency between the different language interfaces (which isn't even necessarily desirable, given the differences in what's idiomatic in different programming languages).

Also, I think the prefered method of setting the state in these cases anyway is

set(w, 'P', pressure, 'Vapor', vapor_fracton);
set(w, T', temperature, 'Vapor', vapor_fracton);

The setState_ functions are only called twice throughout all of of the Matlab examples (once in ignite.m and once in reactor_ode.m).

[speth]

For what it's worth, I did a bit of digging and this bug seems to date back at least to 27c66a8, so this bug's been around for 11 years.

[bryanwweber]

And all it took was a little undergraduate project to find it! With a bit more looking, I think I found the particular commit where this happened: d298e3b#diff-d99fd7840078d17ee086780c97c92389L32

You can see that the case 2 gets reused for the setState_Tsat function, but setState_satLiquid doesn't get removed from the Matlab interface, and keeps using the job number 2. Later, I guess that job number will get moved again, so that setState_satLiquid ended up pointing at the Electric potential function!

[speth]

The history of this bug pretty clearly highlights the main problem with the "magic number" approach to dispatching functions in the Matlab interface.

Assuming that at some point we get around to rewriting the Matlab interface to use the "new" style classes, it would be good to see if there's a way to avoid using this approach.

[bryanwweber]

@speth It seems like the reason this approach was chosen was because it is not possible to have multiple functions defined in the same mex C/C++ file that can be called from the Matlab command line. For example, set, setTemperature, setPressure, etc. would each require their own C++ file with its own mexFunction to work. One alternative might be to directly load the Cantera library into Matlab.

Another approach might be to write a mex file wrapper around the various classes that Cantera uses. There is some discussion of this alternative here, here, and here.

[speth]

Right, I understand why the choice was made to implement it as it is, given the limitations of the mex interface. But the fact that there's no useful way to define meaningful names to integer constants in Matlab gives rise to this unmaintainable parallelism between the constants in the .m files and the constants in the .cpp files.

The idea of just loading the library directly, which would eliminate the single-entry-point limitation imposed by the mex interface, has crossed my mind before and I did a bit of experimentation. One issue which immediately arises is that you can only use C (not C++) functions, so the purported C interface defined in ct.h would need to be fixed so it was actually a C interface and didn't use C++ features like default arguments. Another option would be to write a (hopefully) simple code generator which would implement both sides of the Matlab interface and guarantee consistency. Of course, I'm not particularly interested in volunteering my time to make significant improvements to an interface for an expensive piece of commercial software when I think there are better open source alternatives.

[bryanwweber]

@speth Instead of using magic integers, the examples I linked to use string comparisons in the switch statements, so that you can be mildly more descriptive in the selection of the job to complete. I think that's likely to be the best case for the back end in a mex-type interface, although the front end could certainly be improved by using more modern Matlab OOP techniques.

I'm leery of trying to rewrite the C interface and also worried about the (small?) burden it might put on the end user to have to load the library directly. The code generator sounds like a good idea too.

FWIW, I happen to agree that there are better alternatives to Matlab (Python being the most relevant case for us). However, universities still teach Matlab in their intro CSE course for non-majors, so I think its worth improving the Matlab interface to try and ease the use of Cantera, especially in the classroom. Of course, without any funding for the improvement, its difficult to devote more than an hour here or there...

[speth]

The problem is that the string comparison approach is O(n) in the number of possible function calls while the gigantic switch statement is always just O(1). I think the performance hit of 100 string comparisons just to get e.g. the temperature of a mixture would start to add up.

I figure that you could add the check to make sure that the library was loaded in the constructor call for each class so the user didn't have to do it manually. One other advantage of using a library that doesn't touch the mex interface is that you don't need the Matlab headers in order to compile the interface, so you can build it on a computer without Matlab installed.

I know many universities still teach Matlab to their engineering majors, and I happen to think it's a mistake. Obviously we should strive to have the Matlab toolbox be bug-free, but I think it's perfectly reasonable to have it be less full-featured than the Python interface as a way to encourage users to use Python instead.