-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor full glider metrics #80
Conversation
fcbb8ad
to
83293b8
Compare
@MathewBiddle Mary Solokas require this change to run the glider metrics script I sent her. Do you mind reviewing this one and, after the merge, tag a new release? |
I'm confused on what function is for what. My expectation was ngdac_gliders() is the full search, including providing time and bounding box (see below). However, it seems that ioos_metrics.ioos_metrics.ngdac_gliders(min_time="2023-06-01T00:00:00", max_time="2023-11-30T23:59:59", min_lat=10, max_lat=42, min_lon=-99, max_lon=-50)
Traceback (most recent call last):
File "C:\Users\Mathew.Biddle\programs\Miniforge\envs\ioos-metrics\Lib\site-packages\IPython\core\interactiveshell.py", line 3577, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-7-f4b919fdf3b1>", line 1, in <module>
ioos_metrics.ioos_metrics.ngdac_gliders(min_time="2023-06-01T00:00:00", max_time="2023-11-30T23:59:59", min_lat=10, max_lat=42, min_lon=-99, max_lon=-50)
TypeError: ngdac_gliders() got an unexpected keyword argument 'min_time' I think the second cell in the glider notebook is what's confusing me. https://github.com/ioos/ioos_metrics/blob/main/notebooks/glider_metrics.ipynb Too many things with similar names. Outside of that comment, the rest of these changes look good. |
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.
Everything looks good. Just noticed a syntax difference in defining _make_track_geom()
@@ -206,6 +206,18 @@ def _metadata(info_df) -> dict: | |||
), | |||
} | |||
|
|||
def _make_track_geom(df) -> "pd.DataFrame": |
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.
why is this in quotes when other instances are not?
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.
Both are acceptable but the string version is a trick to avoid importing pandas. Mypy knows that and will check the type regardless.
Some time ago we discussed that the default should be the fast one, that is why the full search is the hidden method. We can change that if you want.
We can make that one a first class citizen and name it |
This is a bit more efficient and we also fix a corner case for when the glider has a single data point in space and we cannot compute a track.