add ranks #268
add ranks #268
Conversation
Codecov Report
@@ Coverage Diff @@
## master #268 +/- ##
=======================================
Coverage 86.71% 86.72%
=======================================
Files 92 92
Lines 4479 4489 +10
=======================================
+ Hits 3884 3893 +9
- Misses 595 596 +1
Continue to review full report at Codecov.
|
| assert rt(df=df, df_exog=df_exog, return_ranks=False, model=DecisionTreeRegressor()).shape == (2, 2) | ||
|
|
||
|
|
||
| def test_relevance_table_ranks(simple_df_relevance): |
There was a problem hiding this comment.
Better to make a fixture for the table with ranks and test the method in two cases of greater_is_better
| @@ -73,7 +73,7 @@ def ts_with_regressors(): | |||
| def test_mrmr_right_len(relevance_method, clustering_method, top_k, ts_with_regressors): | |||
There was a problem hiding this comment.
Better to use keyword arguments, fix it pls in this file
| @@ -59,7 +71,9 @@ class ModelRelevanceTable(RelevanceTable): | |||
| def __init__(self): | |||
| super().__init__(greater_is_better=True) | |||
|
|
|||
| def __call__(self, df: pd.DataFrame, df_exog: pd.DataFrame, **kwargs) -> pd.DataFrame: | |||
| def __call__(self, df: pd.DataFrame, df_exog: pd.DataFrame, return_ranks: bool, **kwargs) -> pd.DataFrame: | |||
There was a problem hiding this comment.
shouldn't we add False as default value for return_ranks?
| @@ -142,6 +142,7 @@ class MRMRFeatureSelectionTransform(Transform): | |||
| def __init__( | |||
| self, | |||
| relevance_method: RelevanceTable, | |||
| return_ranks: bool, | |||
There was a problem hiding this comment.
return_rank sounds like you are going to return smth like {regressor: rank} for chosen top_k, doesn't it? how about use_rank?
and maybe lets make it the last arg (at least after top_k arg)?
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Before submitting (must do checklist)
Type of Change
Proposed Changes
Related Issue
Closing issues