TEOS-10/GSW-Python

Re-implement gsw.infunnel

Opened this issue · 12 comments

ocefpaf commented

Issue TEOS-10/GSW-R#55 kind of reminded me that we had this in https://github.com/TEOS-10/python-gsw/blob/7d6ebe8114c5d8b4a64268d36100a70e226afaf6/gsw/gibbs/library.py#L1535-L1576 but lost it when migrating to the C-wrapped code. Our docs also do not mention this, causing users to open issues like #83.

@efiring would you agree with us re-adding an updated version of infunnel here? We can also add a note in our docs. I'm mentoring a few students and this would be a nice small project for them to send a PR, let me know what you think.

PS: This is the current Matlab version implementation for future reference https://github.com/TEOS-10/GSW-Matlab/blob/master/Toolbox/library/gsw_infunnel.m

dankelley commented

I wonder why there is no check on negative pressures, but in GSW-R I just transliterated the matlab code as-is.

  • 👍1
efiring commented

Good question, though the check should be on negative absolute pressure; "ocean pressure" as defined by GSW can be negative.

ocefpaf commented

@douglasnehme is that something you would like to work on?

efiring commented

I agree that we should have a "funnel" function. The first question, though, is whether it should be added to the C version rather than coded separately in each language.

dankelley commented

I feel that pretty much everything like this ought to be in the C version and then just wrapped in the R, python, etc., versions. This should reduce the error "surface". Also, of course, folks using C directly might want the funnel function.

efiring commented

That's also my inclination: put it in C.

  • 👍1
douglasnehme commented

Yes, @ocefpaf, it would be something I would like to work on. But since @efiring and @dankelley see more gains in implementing the infunnel directly in C we can search for another potential issue.

ocefpaf commented

Sure. I'll find a pure Python project to get your feet wet. With that said, infunnel is not too complicated in you want to try something in C.

douglasnehme commented

Maybe in the future, I try to use C, but now I prefer to give my first contributions to open-source projects with something that I have confidence in.

  • 👍1
ocefpaf commented

This is done on the C-side. We still need to wrap it on the Python side.

@efiring what is the way forward here?

  • Tag and release a new GSW-C or just pull from latest?
  • Do we need to adjust the wrapper here to understand this new function signature?
dankelley commented

A tag is helpful in hinting stability, so that authors of derivative packages can consider updating these packages.

I suspect all packages have documentation that specifies a github hash code. For example, the R version (GSW-R) has the below in its "DESCRIPTION" file. That causes this to be presented at the top of the official CRAN webpage (https://cran.r-project.org/web/packages/gsw/index.html for a note on the package) and the hash code is also shown in the documentation for each function within the package.

Description: Provides an interface to the Gibbs 'SeaWater' ('TEOS-10') C library, version 3.06-16-0 (commit '657216dd4f5ea079b5f0e021a4163e2d26893371', dated 2022-10-11, available at <https://github.com/TEOS-10/GSW-C>, which stems from 'Matlab' and other code written by members of Working Group 127 of 'SCOR'/'IAPSO' (Scientific Committee on Oceanic Research / International Association for the Physical Sciences of the Oceans).
  • 👍1
ocefpaf commented

A tag is helpful in hinting stability, so that authors of derivative packages can consider updating these packages.

I do prefer a tag as well.