Update interval metrics to work with arbitrary interval bounds #113
Conversation
|
🚀 Deployed on https://deploy-preview-113--etna-docs.netlify.app |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #113 +/- ##
==========================================
+ Coverage 89.21% 89.24% +0.03%
==========================================
Files 195 195
Lines 12615 12657 +42
==========================================
+ Hits 11254 11296 +42
Misses 1361 1361
☔ View full report in Codecov by Sentry. |
etna/metrics/intervals_metrics.py
Outdated
|
|
||
| borders_set = {upper_name, lower_name} | ||
| borders_presented = len(borders_set & ts_intervals) == len(borders_set) |
There was a problem hiding this comment.
Why not borders_presented = len(borders_set & ts_intervals) > 0?
What kind of special cases are handled by your code?
There was a problem hiding this comment.
If we have only one of several borders that are presented in the dataset, in that case we pass check, that you sudgesting. Basically, we check here that all provided border names are in the dataset. I think it is more convenient to use issubset method here.
There was a problem hiding this comment.
Will you rewrite this with issubset?
etna/metrics/intervals_metrics.py
Outdated
| super().__init__(mode=mode, metric_fn=dummy, **kwargs) | ||
| self.quantiles = quantiles | ||
| self.quantiles = sorted(quantiles) |
There was a problem hiding this comment.
Don't we want, for example, add validation that len(quantiles) == 2? Or may be make a warning otherwise.
We could also deprecate quantiles parameter for version 3.0.
There was a problem hiding this comment.
Here we have strictly typed that tuple with 2 elements is expected. But we can add a check with error if size of tuple is different.
Will add deprecation of the parameter.
There was a problem hiding this comment.
Let's add the check of the size. Otherwise it can be misleading for the user, I think.
| return ts_train, ts_test | ||
|
|
||
|
|
||
| @pytest.fixture() |
There was a problem hiding this comment.
Let's remove parentheses.
| ((0.025, 0.975), "target_0.025", "target_0.975"), | ||
| ((), "target_0.025", "target_0.975"), | ||
| ((), "target_lower", "target_upper"), | ||
| ((0.1, 0.9), "target_lower", "target_upper"), |
There was a problem hiding this comment.
How is this test case working? As I understood, there are only lower, upper, 0.025 and 0.975 columns.
There was a problem hiding this comment.
So, here we just try different ways to select interval.
- select the same interval with different methods
- disable quantiles and use border names to select quantiles
- disable quantiles and use border names to select other interval
- quantiles point to non-existing interval, but names point to correct
This fixture contains both sets of intervals. It is done to simplify testing procedure, but somewhat artificial.
etna/metrics/intervals_metrics.py
Outdated
|
|
||
| class Coverage(Metric, _QuantileMetricMixin): | ||
| quantiles_set = {f"target_{quantile:.4g}" for quantile in quantiles} | ||
| quantiles_presented = len(quantiles_set & ts_intervals) == len(quantiles_set) |
There was a problem hiding this comment.
Can't we make this with issubset?
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #102