Skip to content

[MRG] Fix RCA_Supervised sklearn compat test #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 13, 2019
Merged

[MRG] Fix RCA_Supervised sklearn compat test #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aba686e
FIX fix RCA_Supervised sklearn compat test
May 2, 2019
2b8437f
Address https://github.com/metric-learn/metric-learn/pull/198#pullreq…
May 9, 2019
aaed9b2
Refactor comment
May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions metric_learn/rca.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
def _chunk_mean_centering(data, chunks):
num_chunks = chunks.max() + 1
chunk_mask = chunks != -1
chunk_data = data[chunk_mask]
# We need to ensure the data is float so that we can substract the
# mean on it
chunk_data = data[chunk_mask].astype(float, copy=False)
chunk_labels = chunks[chunk_mask]
for c in xrange(num_chunks):
mask = chunk_labels == c
Expand Down Expand Up @@ -98,7 +100,7 @@ def fit(self, X, chunks):
When ``chunks[i] == -1``, point i doesn't belong to any chunklet.
When ``chunks[i] == j``, point i belongs to chunklet j.
"""
X = self._prepare_inputs(X, ensure_min_samples=2)
X, chunks = self._prepare_inputs(X, chunks, ensure_min_samples=2)

# PCA projection to remove noise and redundant information.
if self.pca_comps is not None:
Expand All @@ -109,7 +111,6 @@ def fit(self, X, chunks):
X_t = X - X.mean(axis=0)
M_pca = None

chunks = np.asanyarray(chunks, dtype=int)
chunk_mask, chunked_data = _chunk_mean_centering(X_t, chunks)

inner_cov = np.atleast_2d(np.cov(chunked_data, rowvar=0, bias=1))
Expand Down
12 changes: 9 additions & 3 deletions test/test_sklearn_compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,15 @@ def stable_init(self, sparsity_param=0.01, num_labeled='deprecated',
dSDML.__init__ = stable_init
check_estimator(dSDML)

# This fails because the default num_chunks isn't data-dependent.
# def test_rca(self):
# check_estimator(RCA_Supervised)
def test_rca(self):
def stable_init(self, num_dims=None, pca_comps=None,
chunk_size=2, preprocessor=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use **kwargs here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that would be better in general, but for here I think it might be useful to simulate dRCA to have the same arguments names as a real RCA, so that if scikit-learn have checks that depend on the arguments of RCA, they will be taken into account, what do you think ?
Though it's not the case here so maybe yes we can put **kwargs for simplicity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how scikit-learn's testing works, but that seems plausible. This is fine as-is.

# this init makes RCA stable for scikit-learn examples.
RCA_Supervised.__init__(self, num_chunks=2, num_dims=num_dims,
pca_comps=pca_comps, chunk_size=chunk_size,
preprocessor=preprocessor)
dRCA.__init__ = stable_init
check_estimator(dRCA)


RNG = check_random_state(0)
Expand Down