search

TEOS-10 / python-gsw

Python implementation of the Thermodynamic Equation Of Seawater - 2010 (TEOS-10)
MIT License
45 stars 22 forks source link

5 functions fail their tests via check_functions #10

Closed efiring closed 7 years ago

efiring commented 9 years ago

check_functions.py is showing 5 functions with test output that doesn't match the check values:

I suspect that at least the last 4 of these were written based on an earlier Matlab version, and there have been considerable changes in the Matlab code for the freezing point calculations. I don't know whether the problem with enthalpy_second_derivatives is also because of a change between Matlab versions, or something else.

richardsc commented 9 years ago

Hi all -- I'm not a contributor to this repo, but am following as I'm one of the developers of the GSW-R package. I think you're right about the earlier versions for CT_freezing, t_freezing, brineSA_CT, brineSA_t, as if you look at the gsw_version_history those functions were all updated for version 3.04.

efiring commented 9 years ago

I find it very hard--virtually impossible--to figure out from gsw_version_history when real algorithm changes, and changed outputs, have occurred. Looking at enthalpy_second_derivatives, though, it is plain that its implementation is completely different in version 3.04 than in whatever the present python version was based on--probably version 2. All of these will simply have to be re-implemented to match 3.04.

efiring commented 9 years ago

An attempt to substitute a version of enthalpy_second_derivatives based on Matlab 3.04 opens a can of worms--yet more functions that will have to be reimplemented to match 3.04 if we are going to get the whole thing working at that level. This is making the R strategy of wrapping the C look more and more appealing as an alternative--provided the C version remains well maintained.

richardsc commented 9 years ago

Yes, this was one of the primary reasons for our decision to wrap the C code rather than recode everything in R. A few things to point out:

ocefpaf commented 9 years ago

I find it very hard--virtually impossible--to figure out from gsw_version_history when real algorithm changes, and changed outputs, have occurred.

Indeed. Our best option is to keep testing against the latest Matlab source and fix as we go.

This is making the R strategy of wrapping the C look more and more appealing as an alternative--provided the C version remains well maintained.

I know that re-coding is a foolish herculean job. I would be in favor of wrapping if the C (or even the Fortran) version was THE GSW library. But it seems that only the Matlab version is under heavy development and well maintained. BTW there is a python wrapper for the C version:

https://github.com/lukecampbell/gsw-teos

PS1: @PaulMBarker things would be much easier for us if the Matlab version was version controlled and evolved in human "readable" chunks instead of a code dump every new version. Without version controlling we are back to zero every new version.

PS2: Regarding the bug: I am working on them. I have limited internet connectivity now during the holidays, but I will send the PRs as soon as I get back in the office.

efiring commented 9 years ago

@ocefpaf I think you mean https://github.com/lukecampbell/pygsw. I had come across that before, but it doesn't look serious. As far as I can see, it only works for scalar inputs. That's because this is also true of the C version. The R wrapper adds looping code; this would be needed for a reasonable python version as well, and would not be difficult. On the TEOS-10 github site there is now a Javascript (!) version, which was apparently derived from a PHP version (!!), which was derived from a C version, which was derived from the Fortran version, which was derived from the Matlab version... An interesting thing about the present F90 version is that it includes a bash script that takes a first stab at translating the Matlab to Fortran. In any case, I don't think there is any point in spending time on the Python translation until we know that we are working on a part that will not change between 3.04 and 3.05.

efiring commented 9 years ago

Wrong button. I was just going to add that although the F90 version is the most appealing--it is being worked on now, and it is taking advantage of F90 internal vectorization ("elemental" functions and subroutines)--it cannot be wrapped with f2py because f2py doesn't handle the "elemental" keyword. Maybe there are other problems as well, but that's the first big one.

PaulMBarker commented 9 years ago

Oliver translated the new 76 term functions, this is the biggest change that is occurring. Paul.

From: Eric Firing [notifications@github.com] Sent: Monday, December 29, 2014 1:51 PM To: TEOS-10/python-gsw Cc: Paul Barker Subject: Re: [python-gsw] 5 functions fail their tests via check_functions (#10)

@ocefpafhttps://github.com/ocefpaf I think you mean https://github.com/lukecampbell/pygsw. I had come across that before, but it doesn't look serious. As far as I can see, it only works for scalar inputs. That's because this is also true of the C version. The R wrapper adds looping code; this would be needed for a reasonable python version as well, and would not be difficult. On the TEOS-10 github site there is now a Javascript (!) version, which was apparently derived from a PHP version (!!), which was derived from a C version, which was derived from the Fortran version, which was derived from the Matlab version... An interesting thing about the present F90 version is that it includes a bash script that takes a first stab at translating the Matlab to Fortran. In any case, I don't think there is any point in spending time on the Python translation until we know that we are working on a part that will not change between 3.04 and 3.05.

— Reply to this email directly or view it on GitHubhttps://github.com/TEOS-10/python-gsw/issues/10#issuecomment-68228904.

efiring commented 9 years ago

@PaulMBarker Thanks. This also means that we should not bother with anything that involves the 48-term functions because they will go away, correct?

PaulMBarker commented 9 years ago

Do not do the section density and enthalpy, based on the 48-term expression for density http://www.teos-10.org/pubs/gsw/html/gsw_contents.html#6 But do have a look at the programmes Oliver translated, as they are the replacements for that section. Note that specific volume anomaly will change.

The other sections which refer to the 48 term are fine to work on: water column properties, based on the 48-term expression for density neutral properties, based on the 48-term expression for density geostrophic streamfunctions, based on the 48-term expression for density as they just make call to the 48 term polynomial.

In dynamic height I have a couple of changes to make to the code, and a new interpolation programme. Eric, I will send you the new vertical interpolation routine.

Paul.

From: Eric Firing [mailto:notifications@github.com] Sent: Monday, 29 December 2014 5:57 PM To: TEOS-10/python-gsw Cc: Paul Barker Subject: Re: [python-gsw] 5 functions fail their tests via check_functions (#10)

@PaulMBarkerhttps://github.com/PaulMBarker Thanks. This also means that we should not bother with anything that involves the 48-term functions because they will go away, correct?

— Reply to this email directly or view it on GitHubhttps://github.com/TEOS-10/python-gsw/issues/10#issuecomment-68236205.

durack1 commented 9 years ago

Gents, just FYI the matlab version is now sitting in a github repo https://github.com/TEOS-10/GSW-Matlab although @PaulMBarker isn't actively developing using this.. It would be great if version was used in between releases, which would then allow collaborators rewriting code in variants other than matlab could follow incremental (rather than version/release dump) changes..

ocefpaf commented 7 years ago

I am closing all issues in TEOS-10/python-gsw now that https://github.com/TEOS-10/GSW-Python is live.