Simple Impute for timeseries #975
Conversation
Zethson
left a comment
Zethson
left a comment
There was a problem hiding this comment.
Thank you! A bit pedantic but this is supposed to be a role model
agerardy
left a comment
agerardy
left a comment
There was a problem hiding this comment.
I think I understood it :)
| def test_simple_impute_basic(impute_num_edata, strategy): | ||
| edata_imputed = simple_impute(impute_num_edata, strategy=strategy, copy=True) | ||
| _base_check_imputation(impute_num_edata, edata_imputed) | ||
| def test_simple_impute_basic(impute_num_edata, array_type, strategy): |
There was a problem hiding this comment.
Explanation: This test and the following one are specific for the function, which is about imputation here
|
|
||
| @pytest.mark.parametrize("array_type", ARRAY_TYPES_NUMERIC_3D_ABLE) | ||
| @pytest.mark.parametrize("strategy", ["mean", "median", "most_frequent"]) | ||
| def test_simple_impute_3D_edata(mcar_edata, array_type, strategy): |
There was a problem hiding this comment.
Explanation: this test checks that for numeric data, the method can handle 3D data.
There might be combinations of arguments that are not supported, which should be mentioned in the documentation if not obvious.
|
|
||
| @pytest.mark.parametrize("array_type", ARRAY_TYPES_NONNUMERIC) | ||
| @pytest.mark.parametrize("strategy", ["mean", "median", "most_frequent"]) | ||
| def test_simple_impute_3D_edata_nonnumeric(edata_mini_3D_missing_values, array_type, strategy): |
There was a problem hiding this comment.
Explanation: this checks whether the function can also handle non-numeric 3D data.
There might be combinations of arguments that are not supported, which should be mentioned in the documentation if not obvious.
|
|
||
|
|
||
| ARRAY_TYPES = (asarray, as_dense_dask_array) | ||
| ARRAY_TYPES_NUMERIC = ( |
There was a problem hiding this comment.
I suggest these collections of array types for testing, which I hope reduces some repetitiveness
There was a problem hiding this comment.
I wonder whether we should have this here or even in our compat code because we might also use this for the implementations?
PR Checklist
docsis updatedDescription of changes
Fixes #945, see there for a checklist
ehrapy.pp.simple_imputefor timeseries.Technical details
Additional context