SelectFdr has serious thresholding bug #2771

Closed
depristo opened this Issue Jan 19, 2014 · 5 comments

Comments

Projects
None yet
4 participants
@depristo

The current code reads like:

def _get_support_mask(self):
    alpha = self.alpha
    sv = np.sort(self.pvalues_)
    threshold = sv[sv < alpha * np.arange(len(self.pvalues_))].max()
    return self.pvalues_ <= threshold

But this doesn't actually control FDR at all, the correct implementation should have:

    bf_alpha = alpha / len(self.pvalues_)
    threshold = sv[sv < bf_alpha * np.arange(len(self.pvalues_))].max()

Note the k / m term in the equation at:
http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jan 19, 2014

Member

hum...

can you open a PR with a test?

thanks

Member

agramfort commented Jan 19, 2014

hum...

can you open a PR with a test?

thanks

@depristo

This comment has been minimized.

Show comment
Hide comment
@depristo

depristo Jan 20, 2014

Sorry, I don't have a concrete test for it. But that implementation is just not correct, given the docstring saying it does Benjamini-Hochberg correction, as the term m (== len(self.pvalues_) here) is completely missing from the equation.

Sorry, I don't have a concrete test for it. But that implementation is just not correct, given the docstring saying it does Benjamini-Hochberg correction, as the term m (== len(self.pvalues_) here) is completely missing from the equation.

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Jan 20, 2014

Member

If I read correctly the difference is a matter of scale: whether or not
alpha should be normalised by the number of features. So the manner of
selection is correct, but the parameter is not the same as in the
literature.

On 21 January 2014 06:42, Mark DePristo notifications@github.com wrote:

Sorry, I don't have a concrete test for it. But that implementation is
just not correct, given the docstring saying it does Benjamini-Hochberg
correction, as the term m (== len(self.pvalues_) here) is completely
missing from the equation.


Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32790409
.

Member

jnothman commented Jan 20, 2014

If I read correctly the difference is a matter of scale: whether or not
alpha should be normalised by the number of features. So the manner of
selection is correct, but the parameter is not the same as in the
literature.

On 21 January 2014 06:42, Mark DePristo notifications@github.com wrote:

Sorry, I don't have a concrete test for it. But that implementation is
just not correct, given the docstring saying it does Benjamini-Hochberg
correction, as the term m (== len(self.pvalues_) here) is completely
missing from the equation.


Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32790409
.

@depristo

This comment has been minimized.

Show comment
Hide comment
@depristo

depristo Jan 20, 2014

That's 100% correct. But the fundamental goal for SelectFdr vs. SelectFpr
is to have Fdr scale with the number of samples, and that's precisely what
it's not doing right now.

On Mon, Jan 20, 2014 at 3:08 PM, jnothman notifications@github.com wrote:

If I read correctly the difference is a matter of scale: whether or not
alpha should be normalised by the number of features. So the manner of
selection is correct, but the parameter is not the same as in the
literature.

On 21 January 2014 06:42, Mark DePristo notifications@github.com wrote:

Sorry, I don't have a concrete test for it. But that implementation is
just not correct, given the docstring saying it does Benjamini-Hochberg
correction, as the term m (== len(self.pvalues_) here) is completely
missing from the equation.


Reply to this email directly or view it on GitHub<
https://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32790409>

.


Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32792596
.

Mark A. DePristo
mark@depristo.com

That's 100% correct. But the fundamental goal for SelectFdr vs. SelectFpr
is to have Fdr scale with the number of samples, and that's precisely what
it's not doing right now.

On Mon, Jan 20, 2014 at 3:08 PM, jnothman notifications@github.com wrote:

If I read correctly the difference is a matter of scale: whether or not
alpha should be normalised by the number of features. So the manner of
selection is correct, but the parameter is not the same as in the
literature.

On 21 January 2014 06:42, Mark DePristo notifications@github.com wrote:

Sorry, I don't have a concrete test for it. But that implementation is
just not correct, given the docstring saying it does Benjamini-Hochberg
correction, as the term m (== len(self.pvalues_) here) is completely
missing from the equation.


Reply to this email directly or view it on GitHub<
https://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32790409>

.


Reply to this email directly or view it on GitHubhttps://github.com/scikit-learn/scikit-learn/issues/2771#issuecomment-32792596
.

Mark A. DePristo
mark@depristo.com

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Jan 21, 2014

Member

I would write a test by sampling data and count false positive or whatever
corresponds to the FDR threshold

there is already a test for FDR that should be updated:

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/feature_selection/tests/test_feature_select.py#L242

Member

agramfort commented Jan 21, 2014

I would write a test by sampling data and count false positive or whatever
corresponds to the FDR threshold

there is already a test for FDR that should be updated:

https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/feature_selection/tests/test_feature_select.py#L242

ajtulloch added a commit to ajtulloch/scikit-learn that referenced this issue Mar 4, 2014

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.

@amueller amueller added this to the 0.15.1 milestone Jul 18, 2014

amueller added a commit to amueller/scikit-learn that referenced this issue Jan 22, 2015

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.

amueller added a commit to amueller/scikit-learn that referenced this issue Feb 7, 2015

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.

amueller added a commit to amueller/scikit-learn that referenced this issue Feb 7, 2015

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.

amueller added a commit to amueller/scikit-learn that referenced this issue Feb 24, 2015

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.

@amueller amueller closed this in #4146 Feb 24, 2015

rasbt added a commit to rasbt/scikit-learn that referenced this issue Apr 6, 2015

[Feature Selection] Fix SelectFDR thresholding bug (#2771)
From scikit-learn#2771, we were
not correctly scaling the alpha
parameter (http://en.wikipedia.org/wiki/False_discovery_rate#Benjamini.E2.80.93Hochberg_procedure)
with the number of features (== hypothesis).  Thus, the alpha
parameter was not invariant wrt the number of features.

The correction is as suggested in the original issue, and a test has
been added that verifies that for various numbers of features, an
appropriate false discovery rate is generated when using the selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment