Skip to content

Commit 5db4b8e

Browse files
authored
[BUG] unexpected group order change in VariableImportance (#632)
* add regression test * fix group order * update changelog * docsrtring
1 parent 2c4d4d8 commit 5db4b8e

3 files changed

Lines changed: 29 additions & 4 deletions

File tree

CHANGELOG.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ Bug fixes
1919
---------
2020

2121
* Fixed a bug in CLuDL/EncluDL: cloning the internal estimators to avoid them carrying unwanted history. (PR #425)
22+
* Fix the unexpected change in the order of feature groups when a dataframe is used as input (PR #632)
2223

2324
Maintenance
2425
-----------

src/hidimstat/base_variable_importance.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -541,14 +541,14 @@ def fit(self, X, y=None):
541541
j: [j] for j in range(self.n_features_groups_)
542542
}
543543
self._features_groups_ids = np.array(
544-
sorted(self.features_groups_.values()), dtype=int
544+
list(self.features_groups_.values()), dtype=int
545545
)
546546
elif isinstance(self.features_groups, dict):
547547
self.features_groups_ = self.features_groups
548548
self.n_features_groups_ = len(self.features_groups_)
549549
if isinstance(X, pd.DataFrame):
550550
self._features_groups_ids = []
551-
for features_group_key in sorted(self.features_groups_.keys()):
551+
for features_group_key in self.features_groups_:
552552
self._features_groups_ids.append(
553553
[
554554
i

test/test_base_variable_importance.py

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import matplotlib.pyplot as plt
22
import numpy as np
3+
import pandas as pd
34
import pytest
45
from sklearn.cluster import FeatureAgglomeration
5-
from sklearn.linear_model import LassoCV
6+
from sklearn.linear_model import LassoCV, LinearRegression
67

7-
from hidimstat import CluDL, DesparsifiedLasso
8+
from hidimstat import CFI, CluDL, DesparsifiedLasso
89
from hidimstat.base_variable_importance import BaseVariableImportance
910
from hidimstat.statistical_tools.multiple_testing import fdp_power
1011
from hidimstat.statistical_tools.p_values import two_sided_pval_from_pval
@@ -527,3 +528,26 @@ def test_clustered_fwer_selection(rng):
527528
fwer=0.5, n_tests=n_features, two_tailed_test=True
528529
)
529530
assert selection.shape[0] == n_features
531+
532+
533+
@pytest.mark.parametrize(
534+
"n_samples, n_features, support_size, rho, seed, value, signal_noise_ratio, rho_serial",
535+
[(100, 10, 5, 0.5, 0, 1.0, 64.0, 0.5)],
536+
)
537+
def test_feature_groups_order_preserved(data_generator):
538+
"""Regression test to check that the order of the feature groups is
539+
preserved in the output of .importance() even when the keys are not in
540+
the order that would be returned by sorted(groups.keys()).
541+
"""
542+
X, y, important_features, non_important_features = data_generator
543+
X_df = pd.DataFrame(X)
544+
model = LinearRegression()
545+
model.fit(X_df, y)
546+
groups = {
547+
"non_important": non_important_features,
548+
"important": important_features,
549+
}
550+
551+
cfi = CFI(estimator=model, features_groups=groups, random_state=0)
552+
importance = cfi.fit_importance(X_df, y)
553+
assert importance[0] < importance[1]

0 commit comments

Comments
 (0)