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 method for computing harmonic mean #66

Merged
merged 11 commits into from
Apr 10, 2023
Merged

Add method for computing harmonic mean #66

merged 11 commits into from
Apr 10, 2023

Conversation

lsetiawan
Copy link
Member

@lsetiawan lsetiawan commented Apr 6, 2023

Overview

This PR resolves #65. It adds a method for computing harmonic mean from provided start and end depth of a transponder. This also contains changes for renaming transponder -> transducer for instrument at the surface.

@lsetiawan lsetiawan marked this pull request as ready for review April 6, 2023 22:17
@lsetiawan lsetiawan requested a review from carlosgjs April 6, 2023 22:19
@lsetiawan lsetiawan requested a review from johnbdesanto April 6, 2023 22:23
carlosgjs
carlosgjs previously approved these changes Apr 6, 2023
Copy link
Contributor

@carlosgjs carlosgjs left a comment

Choose a reason for hiding this comment

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

Please see minor comments inline ...

src/seagap/harmonic_mean.py Outdated Show resolved Hide resolved
src/seagap/harmonic_mean.py Outdated Show resolved Hide resolved
src/seagap/harmonic_mean.py Outdated Show resolved Hide resolved
src/seagap/harmonic_mean.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@johnbdesanto johnbdesanto left a comment

Choose a reason for hiding this comment

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

A few things:

  1. I think we should change the name of the transponder_wait_time variable since it corresponds to the delay time of the surface platform. It is not associated with the seafloor transponders. "transducer_delay_time" or "surface_platform_delay_time" would be more to the point.

  2. I'm not sure the validator from lines 171-176 is wholly neccessary. Thus far, we have only used surface platforms with delays of 0.0s or 0.1 s, but there is technically no reason why other platforms may have different delays. The current wave gliders have a delay of 0.13s, but I remove it earlier in the procesing chain so that I can simplify a 0.0s delay.

  3. The harmonic_mean script seems like a 1-1 with the fortran script, so I assume it will work. However, I've noticed that scipy has a function called hmean, which is supposed to calculate a harmonic mean and may give us a similar estimate if we weight the sound velocity records by the depth differences. I am curious to see how it compares with the old fortran script.

@lsetiawan
Copy link
Member Author

I think we should change the name of the transponder_wait_time variable since it corresponds to the delay time of the surface platform. It is not associated with the seafloor transponders. "transducer_delay_time" or "surface_platform_delay_time" would be more to the point.

In this PR that change is made. If you go to the Files changed tab above you can see the differences. The left side of the view is what the code used to be and the right side is what it's gonna be. This is essentially what's called a git diff. Anything colored red in the code means that they will be deleted and green is addition.

Also, you can review this guide on PR reviews where you are able to comment directly line by line https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request :)

@lsetiawan
Copy link
Member Author

I'm not sure the validator from lines 171-176 is wholly neccessary. Thus far, we have only used surface platforms with delays of 0.0s or 0.1 s, but there is technically no reason why other platforms may have different delays. The current wave gliders have a delay of 0.13s, but I remove it earlier in the procesing chain so that I can simplify a 0.0s delay.

Thanks for this insight, when I initially set this up, I didn't have that knowledge, so I will remove the validator.

@lsetiawan
Copy link
Member Author

The harmonic_mean script seems like a 1-1 with the fortran script, so I assume it will work. However, I've noticed that scipy has a function called hmean, which is supposed to calculate a harmonic mean and may give us a similar estimate if we weight the sound velocity records by the depth differences. I am curious to see how it compares with the old fortran script.

I am aware of scipy's hmean, but wasn't sure about what weight to use for it, and when I plug in the numbers directly into the scipy's function, it gave different result than the fortran, and the fortran code didn't exactly explain its formula 😬 so I just did a 1:1. Thanks for your insight for the weights. I'll try it out!

@lsetiawan
Copy link
Member Author

@johnbdesanto Trying out the scipy hmean function, I get the following results:

Transponder 1
start depth: -4.0
end depth: -1176.5866
fortran hm: 1481.551
scipy hm: 1481.56

Transponder 2
start depth: -4.0
end depth: -1146.5881
fortran hm: 1481.521
scipy hm: 1481.531

Transponder 3
start depth: -4.0
end depth: -1133.7305
fortran hm: 1481.509
scipy hm: 1481.518

They seem very close and just off by 0.01! Is this an okay difference? What are your thoughts? Thanks!

@johnbdesanto
Copy link
Collaborator

johnbdesanto commented Apr 8, 2023 via email

@lsetiawan
Copy link
Member Author

These are essentially identical estimates given our uncertainty. I think
hmean is perfect for what we need. Glad to see they agreed so well!

Great! I will go ahead and convert the compute function to use the scipy's hmean! I really appreciate your inputs. 😄 Teamwork makes the dream work! 🥇

@lsetiawan
Copy link
Member Author

@johnbdesanto This is ready for another round of review from you. Thanks!

@lsetiawan
Copy link
Member Author

@carlosgjs Do you have any other suggestions, I've changed a couple of things since your last review. Thanks!

@carlosgjs
Copy link
Contributor

@carlosgjs Do you have any other suggestions, I've changed a couple of things since your last review. Thanks!

LGTM!

@lsetiawan lsetiawan merged commit fef23af into seafloor-geodesy:main Apr 10, 2023
@lsetiawan lsetiawan deleted the hm branch April 10, 2023 19:54
lsetiawan added a commit that referenced this pull request Dec 4, 2023
* feat: Add numba harmonic mean implementation

Added '_sv_harmon_mean' numba implementation for
calculating harmonic mean that uses calculations
used within the original fortran code to get same
harmonic values. Now there's a 'method' value for the
main 'sv_harmonic_mean' function that allows user
to switch back and forth b/w the two methods.

---

Ref #193

* test(harmonic_mean): Separate scipy and fortran aka numba hmean test

Separate out value testing for the two methods of computing
harmonic mean. The fortran values are obtained from
previous discussions at #66 (comment).

* test: Add test for _sv_harmon_mean

Add simple test for using the new '_sv_harmon_mean'
function on dummy arrays.

* test: update raised error to KeyError for missing col

* refactor: Add NotImplementedError for other methods

Added a raise NotImplementedError for any other
harmonic mean methods that is neither "scipy"
nor "numba".

* chore: Apply suggestions from code review

Co-authored-by: carlosgjs <[email protected]>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: carlosgjs <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

Redevelop proc_harmonic_mean_2_sfg_file.csh for computing harmonic mean
3 participants