Update aggregate_metrics_df to work with None values
#522
Conversation
|
🚀 Deployed on https://deploy-preview-522--etna-docs.netlify.app |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #522 +/- ##
==========================================
+ Coverage 90.32% 90.35% +0.02%
==========================================
Files 256 256
Lines 17227 17267 +40
==========================================
+ Hits 15561 15601 +40
Misses 1666 1666 ☔ View full report in Codecov by Sentry. |
etna/metrics/utils.py
Outdated
| def mean_agg(): | ||
| """Mean for pandas agg.""" | ||
|
|
||
| def func(x): |
There was a problem hiding this comment.
Could we add typing here for clarity?
There was a problem hiding this comment.
Here it is probably pd.Series, but during debugging I saw how plain float were passed into this function (as I understand it leads to error and after this pandas passes the whole pd.Series inside).
tests/test_auto/test_auto.py
Outdated
| callback=callback, | ||
| ) | ||
|
|
||
| # TODO: discuss the error here |
There was a problem hiding this comment.
We should discuss this: it fails before we go into metrics computation.
There was a problem hiding this comment.
Seems like we should support this behavior now. We can leave it as is for this task and create a separate issue to change this. What do you think?
There was a problem hiding this comment.
Or we can try to mock here the part that blocks us from testing intended behavior.
tests/test_auto/test_tune.py
Outdated
| callback=callback, | ||
| ) | ||
|
|
||
| # TODO: discuss the error here |
There was a problem hiding this comment.
We should discuss this: it fails before we go into metrics computation.
| ("metrics_df_with_folds_with_missing", "aggregated_metrics_df_with_missing"), | ||
| ("metrics_df_no_folds_with_missing", "aggregated_metrics_df_with_missing"), | ||
| ("metrics_df_with_folds_with_full_missing", "aggregated_metrics_df_with_full_missing"), | ||
| ("metrics_df_no_folds_with_full_missing", "aggregated_metrics_df_with_full_missing"), |
There was a problem hiding this comment.
Why did you remove these tests?
There was a problem hiding this comment.
I just rearranged the tests. I put all of the possible cases in a single fixture.
tests/test_auto/test_auto.py
Outdated
| callback=callback, | ||
| ) | ||
|
|
||
| # TODO: discuss the error here |
There was a problem hiding this comment.
Seems like we should support this behavior now. We can leave it as is for this task and create a separate issue to change this. What do you think?
tests/test_auto/test_auto.py
Outdated
| callback=callback, | ||
| ) | ||
|
|
||
| # TODO: discuss the error here |
There was a problem hiding this comment.
Or we can try to mock here the part that blocks us from testing intended behavior.
Before submitting (must do checklist)
Proposed Changes
See #518.
Closing issues
Closes #518.