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

Re-implement gsw.infunnel #111

Open
ocefpaf opened this issue Oct 17, 2022 · 12 comments
Open

Re-implement gsw.infunnel #111

ocefpaf opened this issue Oct 17, 2022 · 12 comments

Comments

@ocefpaf
Copy link
Member

ocefpaf commented Oct 17, 2022

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
Copy link

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

@efiring
Copy link
Member

efiring commented Oct 17, 2022

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

@ocefpaf
Copy link
Member Author

ocefpaf commented Oct 18, 2022

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

@efiring
Copy link
Member

efiring commented Oct 18, 2022

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
Copy link

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
Copy link
Member

efiring commented Oct 18, 2022

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

@douglasnehme
Copy link

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
Copy link
Member Author

ocefpaf commented Oct 18, 2022

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
Copy link

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.

@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 11, 2024

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
Copy link

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).

@ocefpaf
Copy link
Member Author

ocefpaf commented Jun 11, 2024

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.

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

4 participants