BioSTEAMDevelopmentGroup / thermosteam

BioSTEAM's Premier Thermodynamic Engine
Other
57 stars 12 forks source link

`_chemical.Tsat()` returns boiling point > T critical #64

Closed BenPortner closed 2 years ago

BenPortner commented 2 years ago

Dear @yoelcortes,

I'm starting to feel bad for opening another issue 😅 I hope you don't mind. Thanks for your patience.

_chemical.Tsat(P) returns values, which are larger than the critical temperature for P>Pcrit. For example:

import thermosteam as tmo
tmo.Chemical("H2O").Tsat(P=300e5) # returns 662, Tc = 647

Expected behavior

The saturation temperature cannot be higher than the critical temperature. _chemical.Tsat should probably throw an error or at least a warning.

Proposed solution

_chemical.Tsat uses flexsolve.open_solvers.aitken_secant under the hood. The correct Tmax is passed in its call but aitken_secant does not throw an error, even when setting checkroot=True and checkiter=True. Therefore, we would probably have to check for Tsat<Tmax manually in _chemical.Tsat before returning the result.

I am not sure if throwing an error would break any other functions (perhaps errors are suppressed to allow iterative optimization in other places in the code). If this is the case, then throwing a warning would be sufficient.

yoelcortes commented 2 years ago

Hi Ben,

I'm happy to see you post issues and glad to see you actively using the software. The reason why Tsat does not throw an error is because Psat allows for extrapolation. All TDependentProperty objects (including Psat) are from the Caleb's thermo. You can avoid extrapolation of Psat by setting <Chemical>.Psat.extrapolation = None. For enhancements to the thermo library (warning messages or better error messages), please submit an issue to https://github.com/CalebBell/thermo. I am also a contributor there and I help out in resolving issues too. I would personally avoid warnings in this case (just let it extrapolate) because it is a "hot" path and can lead to lot's of warnings and some overhead.

Yeah, removing extrapolation would break the code in some places and slow down iteration is others. One example is in VLE when working with chemicals with vastly different critical points. I believe other models for VLE such as PSRK would be able to avoid extrapolations. This is within my goals, but I probably cannot get to it this year.

Thanks!

BenPortner commented 2 years ago

Hi @yoelcortes,

Thank you as always for the quick answer and in-depth explanation. I understand that throwing errors or warnings in _chemical.Tsat is undesirable because it would break existing functionality or at least affect performance. However, I think that in-experienced users like me should be warned when they use _chemical.Tsat for their own purpose and the returned result is wrong. I would like to propose a solution, which should satisfy both demands:

Throwing an error is switched on per default (such that users will encounter an error when using _chemical.Tsat directly), but the error is suppressed when_chemical.Tsat is used internally in thermosteam (such that performance and overhead of internal functions like VLE are not affected):

    def Tsat(self, P, Tguess=None, Tmin=None, Tmax=None, check=True): # checking the result is turned on by default
        ...
        if y0 < 0. < y1:
            T = IQ_interpolation(lambda T: Psat(T) - P,
                                 Tmin, Tmax, y0, y1,
                                 Tguess, 1e-6, 1e-2, checkroot=False)
        else:
            T = aitken_secant(lambda T: Psat(T) - P,
                              Tmin, Tmax, 1e-6, 1e-2, checkroot=False)
        # check result before returning
        if check is True:
            if T > Tmax or T < Tmin:
                raise ValueError("...")
        return T

Internal use suppresses the check actively, e.g. in equilibrium.vle.VLE:

...
    def _set_PV_chemical(self, P, V):
        # Set vapor fraction
        self._T = self._thermal_condition.T = self._chemical.Tsat(P, check=False) # check switched off
...

What do you think?

yoelcortes commented 2 years ago

Yeah, there are only a few places where Tsat is used, so this kind of API works and is easy to implement. For other properties, you'll need to set <TDependentProperties>.extrapolate=False (for now at least). I went ahead and added a check for the value of P (should be under Pc): https://github.com/BioSTEAMDevelopmentGroup/thermosteam/blob/d15539e5952d95bee9aadd74458dbc9cdd37619f/thermosteam/_chemical.py#L1299

Thanks!