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

Add gsw_sa_ct_interp and gsw_tracer_ct_interp #73

Merged
merged 29 commits into from
Sep 25, 2024

Conversation

mauzey1
Copy link
Contributor

@mauzey1 mauzey1 commented Sep 23, 2024

This adds C versions of the GSW-Matlab functions gsw_SA_CT_interp and gsw_tracer_CT_interp.

I plan to add these functions to GSW-Python in a followup PR.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nice piece of work--and a lot of work! I've done some cursory checks, but I am relying largely on the tests. The PR could be accepted at this point, but for completeness, please consider adding support for the new interpolation to gsw_geo_strf_dyn_height_1, either in this PR or a follow-up:

#define INTERP_METHOD_MRST 3

and then a corresponding block in the function. We will want this in the long run; it makes sense to have the option to use different interpolation methods in the dynamic height calculation. MRST makes sense for vertically coarse bottle data, but not for relatively well-resolved data. Even where it does make sense, it can be advantageous to be able to compare the MRST result to simpler versions.

@efiring
Copy link
Member

efiring commented Sep 25, 2024

To be clear: if you would prefer to leave the requested change for a separate PR, I would be willing to merge the current PR first. The complexity would all come in the test implementation, not in the additional define and the matching code block in the function.

@mauzey1
Copy link
Contributor Author

mauzey1 commented Sep 25, 2024

@efiring I'll consider adding the new interpolation to gsw_geo_strf_dyn_height_1 in another PR.

@efiring efiring merged commit e6afedf into TEOS-10:master Sep 25, 2024
11 checks passed
@efiring
Copy link
Member

efiring commented Sep 25, 2024

@mauzey1 Thank you for this large contribution. I look forward to follow-ups here and in GSW-Python. Out of curiosity: what motivated you to start working on GSW?

@mauzey1 mauzey1 deleted the add_gsw_sa_ct_interp branch September 25, 2024 22:07
@durack1
Copy link

durack1 commented Oct 7, 2024

@efiring I can answer there, @mauzey1 is working with me at LLNL, and we had a small amount of project funds that could be used to attempt to bring some of the GSW-matlab functionality across to the license-free GSW variants. The aim was to tackle a number of the functions that didn't exist in license-free software, so hopefully this makes a start in that direction

@efiring
Copy link
Member

efiring commented Oct 7, 2024

@durack1 Thank you, it's a great start--a big contribution.

@durack1
Copy link

durack1 commented Oct 7, 2024

@efiring that thanks should go directly to @mauzey1 - we're lucky to have him

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

Successfully merging this pull request may close these issues.

3 participants