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

kinematics.spin and kinematics.eddy_kinetic_energy #307

Merged
merged 16 commits into from
Nov 2, 2023

Conversation

milancurcic
Copy link
Member

@milancurcic milancurcic commented Oct 24, 2023

  • eddy_kinetic_energy:
    • implementation
    • docs
    • tests
  • spin:
    • implementation
    • docs
    • tests

@milancurcic milancurcic added enhancement New feature or request archive-label-analysis-functions Oceanographic Lagrangian analysis functions labels Oct 24, 2023
@selipot
Copy link
Member

selipot commented Oct 25, 2023

The formula reference should be in Griffa et al. 2008

@milancurcic milancurcic changed the title kinematics.spin kinematics.spin and kinematics.eddy_kinetic_energy Oct 26, 2023
@milancurcic
Copy link
Member Author

@selipot take a look at the current implementation.

clouddrift/kinematics.py Outdated Show resolved Hide resolved
@@ -667,3 +724,147 @@ def velocity_from_position(
dy /= dt

return np.swapaxes(dx, time_axis, -1), np.swapaxes(dy, time_axis, -1)


def spin(
Copy link
Member

Choose a reason for hiding this comment

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

Are we sorting alphabetically functions by convention? Just wondering.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we've had a rule so far. I think it would be nice for functions to be sorted alphabetically in the code (easier to find without resorting to search).

clouddrift/kinematics.py Outdated Show resolved Hide resolved
Parameters
----------
u : np.ndarray
Zonal velocity
Copy link
Member

@selipot selipot Oct 26, 2023

Choose a reason for hiding this comment

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

I always end with a period as in

Zonal velocities.

Should we have a convention?

Copy link
Member Author

Choose a reason for hiding this comment

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

The pedantically correct thing to do (as I learned from my book editor) is to use a period when we have a full sentence (e.g. "This is a zonal velocity.") and to omit it when it's not a sentence ("Zonal velocities").

We don't need to do that because almost nobody will care. We can use periods always if you prefer it.

clouddrift/kinematics.py Show resolved Hide resolved
@selipot
Copy link
Member

selipot commented Oct 31, 2023

Add DOIs to references?

@selipot
Copy link
Member

selipot commented Nov 1, 2023

Another implementation @RickLumpkin reminded me of https://agupubs.onlinelibrary.wiley.com/doi/10.1002/2015JC011435

@milancurcic milancurcic marked this pull request as ready for review November 2, 2023 17:44
@milancurcic
Copy link
Member Author

This is now ready for another review. It bumps the version to 0.26.0.

@milancurcic milancurcic merged commit 6560af4 into Cloud-Drift:main Nov 2, 2023
12 checks passed
@milancurcic milancurcic deleted the spin branch November 2, 2023 18:00
philippemiron pushed a commit to philippemiron/clouddrift that referenced this pull request Nov 16, 2023
* first spin implementation

* Implement eddy_kinetic_energy and spin

* edit

* EKE description and spin references

* Fix change introduced in bad merge from main

* Test eddy_kinetic_energy

* Update kinetic_energy and spin to not demean

* TODO comment

* Update spin docstring

* Fix docstring

* Don't time average in spin calculation

* Bump version

* Update docstrings

* Test spin

---------

Co-authored-by: selipot <selipot@miami.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archive-label-analysis-functions Oceanographic Lagrangian analysis functions enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants