Skip to content
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

Given valid inputs, some functions return values outside of valid ranges #83

Open
FudgeMunkey opened this issue Nov 23, 2021 · 3 comments

Comments

@FudgeMunkey
Copy link

FudgeMunkey commented Nov 23, 2021

Bug

I have been writing Property Based Tests for GSW using the Hypothesis Framework. I have tried to provide reasonable inputs and outputs for these functions, however they sometimes return values outside of this range.

The following input ranges were used:

Parameter Minimum Maximum
Salinity (g/kg) 0 42
Temperature (C) -2 40
Ice Temperature (C) -20 0
Sea Pressure (dbar) 0 11,000
Fractions 0 1

The following output ranges were used:

Parameter Minimum Maximum
Salinity (g/kg) 0 500
Temperature (C) -10 60
Ice Temperature (C) -30 10

The following functions are affected:

  • melting_ice_into_seawater
  • pt0_from_t_ice
  • SA_freezing_from_CT
  • SA_freezing_from_CT_poly
  • SA_freezing_from_t

Resources

I used these resources to generate the input ranges. If they are incorrect, please let me know and I can adjust my tests and rerun.

  1. Key Physical Variables in the Ocean: Temperature, Salinity, and Density (2010)
  2. Accurate polynomial expressions for the density and specific volume ofseawater using the TEOS-10 standard
  3. Algorithms for Density, Potential Temperature, Conservative Temperature, and the Freezing Temperature of Seawater
  4. Three-stage vertical distribution of seawater conductivity
  5. Documentation

To Reproduce

I have added tests to this branch and added Hypothesis as a dependency to requirements-dev.txt. Clone the repo, checkout the branch, install the depedencies and run the tests using pytest as normal.

I have also listed falsifying examples for each of the functions below.

melting_ice_into_seawater(SA=33.588601420681144, CT=-1.5373627366686948, p=8797.014657797441, w_Ih=0.7997286052543129, t_Ih=-15.373627366686918) 
# -10 <= -10.000000000000002

pt0_from_t_ice(t=10.000000000000002, p=0.0) 
# 10.000000000000002 <= 10

SA_freezing_from_CT(CT=0.0, p=24.056326027857683, saturation_fraction=0.06250937321146123) 
# 0 <= -4.662438729975404e-13

SA_freezing_from_CT_poly(CT=0.0, p=24.056384375784173, saturation_fraction=0.06251032159023075) 
# 0 <= -1.1594108914858864e-17

SA_freezing_from_t(t=0.0, p=2.381246187626918, saturation_fraction=0.31252766369605284) 
# 0 <= -1.1092645003440169e-15

To find examples in melting_ice_into_seawater running Hypothesis, you may to increase the number of max examples. This can be done by adding the decorator @settings(max_examples=25000) before the test_melting_ice_into_seawater function.

Please note that many of the parameters are 0.0 because Hypothesis tries to shrink falsifying examples. All of the functions (except for sometimes melting_ice_into_seawater) are still able to find falsifying examples even if the min/max values are +/- 0.1.

Expected behaviour

I would expect the functions to always return scalar values within the output ranges.

Environment

  • Ubuntu 20.04
  • Python 3.8.10
  • GSW 3.4.1.post1.dev1+gcede756.d20211122
@richardsc
Copy link

Hi! I'm not strictly one of the python devs, but since the python library bundles the same C library that R does I'm interested in what's going on here.

For this case, I would point out that your input conductivity range is very wrong -- seawater conductivity in mS/cm should be in the range of 0-60 or so.

I'll let the python devs comment specifically on the tests and we can see if the underlying C functions need to be fixed (or potentially the Matlab originals).

@FudgeMunkey
Copy link
Author

Thank you for letting me know about the seawater conductivity ranges, I've updated the tests and it's no longer a problem for SP_from_C in this issue. It does return NaN sometimes though, so I'll update #82 to reflect this.

@DocOtak
Copy link
Contributor

DocOtak commented Nov 24, 2021

First off, love the idea of hypothesis and have never really been able to find ways to incorporate it into my own testing stuff.

Here is my impression of these results so far...

  • melting_ice_into_seawater:
    this is melting a parcel of water that is almost 80% -15C ice at ~8000 meters depth. The melting point of water drops quite a bit at these pressures. At full ocean depth I think the melting point is around -10C for pure water alone, lower it another 2C probably for salinity and I'd expect around -12C to be the lower limit of liquid water in the ocean (back-of-the-envelope calculation caveat here)
  • pt0_from_t_ice:
    It looks like this was passed an in situ seawater temperature where the function expect the temperature of the ice, then checked again the expected range for ice. For this function, pt0_from_t_ice(t, p=0) is just equal to t and the function doesn't care if this it is realistic at all.
    # temperature of the universe ~100s after the big bang
    >>> gsw.pt0_from_t_ice(1e9,0)
    1000000000.0
  • SA_freezing_from_CT and friends SA_freezing_from_CT_poly and SA_freezing_from_t:
    Two things jump out at me for these, the first is that the results should definitely not be negative (it is supposed to return a nan if SA would be negative). The other thing is that these results are suspiciously right around the machine epsilon for double precision numbers and I know there is at least one round of Newton's method in these functions. In the existing tests, these functions are considered accurate only to about 2.1e-11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants