Closed ghost closed 7 years ago
Great @okrzysik!
Obviously the format for of each docstring is incorrect, these are just the MATLAB comments with a couple of slight alterations.
I've done a little testing on each of the definitions by comparing the output with that of MATLAB. I haven't picked up on any issues except with CT_from_rho.
The issues I am having with CT_from_rho stem from using matrix style input. Sometimes it returns a numerical value where it should have returned a NaN. And sometimes it returns an error saying I have tried to index something out of bounds. I tried to resolve both of these but I didn't have much luck. CT_from_rho is very similar to that of the existing t_from_rho_exact. So perhaps it would just be best to reuse t_from_rho_exact but make the couple of necessary changes to convert it to CT_from_rho.
I have written a definition for CT_freezing_poly as CT_from_enthalpy calls it internally. However, I'm not really sure I needed to do this as CT_freezing_poly does the same thing as the already implemented CT_freezing (I think!), it's just had a name change. I just thought I would be consistent with the MATLAB code -- but this was probably unnecessary in this case.
@okrzysik That's a big chunk all at once! I have only had time for a quick look at parts of it. It will need some changes. I think the best way to handle it would be to start with a PR that only has the first few functions in the module--very few. This way, we can use the line-comment facility of github, which is disabled when such a large file is added. In addition, it means that by reviewing a small chunk of code, we can point out the sorts of things that can be improved; then you can incorporate that knowledge in the next few functions in a subsequent PR, etc. Some things are immediately apparent by comparison with thermodynamics_from_t.py:
cond
), it's enough to use if cond:
or if not cond:
; we don't need == True
etc.It's great that you are off and running with this!
That's a big chunk all at once! I have only had time for a quick look at parts of it.
@efiring that is my fault! @okrzysik was ending his work with Paul and I encourage him to submit what he had.
It will need some changes.
Indeed. Lets leave this PR un-merged for a while and I will break it down to smaller chunks of code and create new PRs. (or I will guide @okrzysik if he wishes to continue working on this.)
@ocefpaf my intention was to continue working on this after I finished my work with Paul, but I have been quite busy with some other things and haven't had a chance to look at it again yet. I will hopefully have a little more spare time from now on, so I would be happy work on it under your guidance.
@okrzysik great to hear that you want to continue work on this. Take your time though, slow and steady wins the race :wink:
Meanwhile I will break @efiring suggestions down to small pieces that we can tackle with small PRs.
There are errors on some of the functions here, thus inconsistent with the GSW-Matlab 3.0.5 output.
Closing this b/c we are going the c-wrapper route.
New python definitions corresponding to the 76 term expression for specific volume.