Minor improvements to type checking and error checks in stats.py. #1493
Conversation
Add a missing error check on summary()'s kind parameter, improve type hints to use explicit Literal types instead of str, and resolve a type error caused by unintentional variable-sharing across unrelated function blocks.
| @@ -1322,12 +1329,12 @@ def summary( | |||
| dfs = [] | |||
| for var_name, values in joined.data_vars.items(): | |||
| if len(values.shape[1:]): | |||
| metric = list(values.metric.values) | |||
There was a problem hiding this comment.
This metric assignment clashed with the metric assignment on line 1307 in the original file: for metric, circ_stat in zip(...). This was a type error since the two assignments to metric had different types.
Renaming this metric to index_metric resolves the problem since now the differently-typed values are assigned to different variables.
| @@ -1302,7 +1310,6 @@ def summary( | |||
| metric_names.extend(metrics_names_) | |||
|
|
|||
| if circ_var_names: | |||
|
|
|||
There was a problem hiding this comment.
This was extraneous whitespace that did not seem to match the style of the surrounding code, so I removed it — hope you don't mind!
| if extend: | ||
| metrics_names_ = ( | ||
| metrics_: Tuple[xr.Dataset, ...] | ||
| metrics_names_: Tuple[str, ...] = ( |
There was a problem hiding this comment.
The block below is why I noticed that the kind error check was missing, and added it: if kind is not one of "all", "stats", "diagnostics", then metrics_names_ will remain set to the tuple of strings, but metrics_ will never get assigned — even though it's about to get used at the end of the if-elif-elif block. This would have led to a user-unfriendly UnboundLocalError.
With the error check I added, supplying an illegal kind value is treated the same way as supplying an illegal order or fmt value: with a TypeError containing a descriptive and actionable error message.
Codecov Report
@@ Coverage Diff @@
## master #1493 +/- ##
==========================================
- Coverage 91.97% 91.96% -0.01%
==========================================
Files 105 105
Lines 11239 11244 +5
==========================================
+ Hits 10337 10341 +4
- Misses 902 903 +1
Continue to review full report at Codecov.
|
|
LGTM, I have added an extra test and will merge if it passes to try and release this week. |
Description
summary()function'skindparameter. Since the check was missing, that meant that illegalkindvalues could be passed, and cause unexpected, self-inconsistent behavior in the function.Literaltypes instead ofstr, making errors like the ones mentioned in the prior bullet point less likely to happen asmypywould catch them statically.As with my other PRs, there are no substantive changes to existing functionality and there is no new functionality, so existing tests should cover everything already.
Checklist