Add MRMR #439
Add MRMR #439
Conversation
Codecov Report
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
+ Coverage 87.63% 87.75% +0.11%
==========================================
Files 112 114 +2
Lines 5275 5334 +59
==========================================
+ Hits 4623 4681 +58
- Misses 652 653 +1
Continue to review full report at Codecov.
|
|
|
||
| def __init__( | ||
| self, | ||
| relevance_method: RelevanceTable, |
There was a problem hiding this comment.
could you please rename relevance_method -> relevance_table?
|
|
||
|
|
||
| class MRMRFeatureSelectionTransform(BaseFeatureSelectionTransform): | ||
| """Transform that selects regressors according to MRMR variable selection method.""" |
There was a problem hiding this comment.
This is not quite a mrmr method, but a slightly modified one. Maybe so and write that "modified mrmr" so that no one gets confused?
There was a problem hiding this comment.
What do you mean under "modifications"?
| regressors=ts[:, :, ts.regressors], | ||
| top_k=self.top_k, | ||
| relevance_aggregation_mode=self.relevance_aggregation_mode, | ||
| redundancy_aggregation_mode=self.redundancy_aggregation_mode, |
There was a problem hiding this comment.
the atol parameter is not used in any way, can it be removed from the mrmr function?
There was a problem hiding this comment.
It can't be removed because we use it calculations, the possible solutions are:
- Pass it as a parameter to the transform
- Create a constant for it(in the early implementation we decided to avoid using it as a constant and put it in the list of parameters)
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
closes #419