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

Thull dive number depth #134

Merged

Conversation

tomhull
Copy link
Contributor

@tomhull tomhull commented Jul 14, 2021

The current calc_dive_phase and related functions are not suitable for shelf sea work as they assume valid dives are deeper than 200 m.
This pull request adds new threshold variable to the minimum valid dive depth can be adjusted. The default has been changed to 15 m which feels deep enough to avoid heavy swell.
Additionally, I’ve included a test for calc_dive_number.

Copy link
Contributor

@jbusecke jbusecke left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the great contribution @tomhull.

I had some minor comments about the defaults and a suggestion for extending the test. Should be ready to merge very soon.

glidertools/utils.py Show resolved Hide resolved
glidertools/utils.py Outdated Show resolved Hide resolved
glidertools/utils.py Show resolved Hide resolved
tests/test_dive_numbers.py Show resolved Hide resolved
@jbusecke
Copy link
Contributor

Sorry for the delay @tomhull, I was on vacation. Lets check back on this when the test have run.

@soerenthomsen
Copy link
Member

@tomhull and @jbusecke this looks like it is almost done or?

@jbusecke
Copy link
Contributor

Definitely needs to be rebased and we should wait for a fix to #149

@MartinMohrmann
Copy link
Member

@tomhull Thanks for the contribution and for including a test! :)
Please remove the unnecessary import of pytest from the file test_dive_numbers.py though. See the failed code-style test result.

@MartinMohrmann MartinMohrmann dismissed jbusecke’s stale review July 18, 2023 09:06

Why are you dismissing this review? The discussion about the minimum valid dive depth has been here for over two years now. I believe this should no longer be a reason to block the nice contribution of @tomhull to be finally merged.

Copy link
Member

@MartinMohrmann MartinMohrmann left a comment

Choose a reason for hiding this comment

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

The provided change to make the dive_depth_threshold adjustable is essential for shelf/coastal seas. Tests are provided and working.

@MartinMohrmann MartinMohrmann merged commit 9d5a6ca into GliderToolsCommunity:master Jul 18, 2023
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.

4 participants