-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation
New enthalpy_SSO_0, consistent with the GSW-3.0.5
ocefpaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efiring are you OK merging this or are you committed to the C-wrapped version?
|
@efiring @ocefpaf I have no problem to later replace the functions that I'm submitting by a C-wrapped version, i.e. there is no long term commitment if approve these PRs. This case specifically doesn't interfere with the existent code, since enthalpy_SSO_0() is a new function, thus is not used in the current code, but only in the 3.0.5 version. |
efiring
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, apart from my question about numpydoc. This is also the sort of function that could be added to the C version very easily.
gsw/gibbs/library.py
Outdated
| array([ 97.26388583, 486.27439853, 1215.47517122, 2430.24907325, | ||
| 5827.90879421, 9704.32030926]) | ||
| Version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does numpydoc handle this "Version" heading? I don't see it in https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not try to make both version live together in master. We have a release for the old one. If this is the path you guys want to go I'd say remove all the 48 terms parts and start coding the 75 here.
We can create a maintenance branch for the 48 term if too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; I agree, we need separate branches for 48 term (which is matlab v3.04?) and 75-term (matlab v3.05).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create the 48 matlab (v3.04) now and we can use master as the latest 75-term. Is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review the doc.
I would suggest creating another branch for 75 as well, so I PR into the 75 instead of master during development. Whenever it get consistent, we merge 75 into master. Despite outdated, master (48) is still the stable distro.
|
On 2017/03/13 10:57 AM, Filipe wrote:
I will create the 48 matlab (v3.04) now and we can use |master| as the
latest 75-term. Is that OK?
Yes, thank you.
|
Done. See https://github.com/TEOS-10/python-gsw/tree/48-term
I'd rather not. All projects I know and contribute use |
New enthalpy_SSO_0, consistent with the GSW-3.0.5
doctest values came from GSW-Matlab