Add warnings and raised errors for add_location
#1296
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1296 +/- ##
==========================================
+ Coverage 83.52% 91.66% +8.14%
==========================================
Files 64 3 -61
Lines 5686 168 -5518
==========================================
- Hits 4749 154 -4595
+ Misses 937 14 -923
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Oh wait, I'll add some tests to cover these scenarios. |
|
@leewujung This PR should be ready for review now |
ctuguinay
added
bug
echopype/consolidate/api.py
Outdated
|
|
||
| # Check if there are duplicates in time_dim_name | ||
| if len(np.unique(echodata["Platform"][time_dim_name].data)) != len( | ||
| echodata["Platform"][time_dim_name].data | ||
| ): | ||
| logger.warning( | ||
| f'The echodata["Platform"]["{time_dim_name}"] array contains duplicate values. ' | ||
| "Interpolation expects unique time values." | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think this should raise an error instead of a warning? Because the underlying code would actually raise a warning. This is as opposed to the GPS NaN/0 cases that the underlying code would run through without erroring out.
There was a problem hiding this comment.
Ah, I see. I think I misinterpreted your original statement here: #1294 (comment). I assumed that meant letting Pandas call the error and we provide a warning message prior to showing the user where the error occurred
ctuguinay
changed the title
add_locationadd_location
|
@leewujung Made the switch from warning to raised value error. This should be ready for a quick review again. |
| @@ -204,7 +204,7 @@ def sel_interp(var, time_dim_name): | |||
| ).any() | |||
| interp_msg = ( | |||
| "Interpolation may be negatively impacted, " | |||
| "consider handling these values before calling add_location." | |||
| "consider handling these values before calling ``add_location``." | |||
There was a problem hiding this comment.
Do you know if the error/earning messages would be formatted if one uses backticks ? I know it would be rendered in docstrings, but not sure about the printout messages. I guess this is useful regardless because users would know the thing in the quote is a variable?
Add warnings for
add_locationfor NaN/0 lat/lon and duplicated timestamps to address the following issues:Strange GPS coordinates appearing with add_location() method #1286
add_location fails with duplicated timestamps in GPS data #1294