[GSK-1279] Fisher's exact test and permutation test for slice metrics significance testing #1671
Conversation
… significance testing
mattbit
left a comment
mattbit
left a comment
There was a problem hiding this comment.
Few minor comments but otherwise looks good to me.
| perm_slice_dataset = Dataset( | ||
| dataset.df.loc[slice_ids], | ||
| target=dataset.target, | ||
| column_types=dataset.column_types.copy(), |
There was a problem hiding this comment.
I don't think we need to make a copy here
| def _calculate_pvalue_from_permutation_test( | ||
| slice_dataset, comp_dataset, dataset, model, metric, perm_test_resamples=1000 | ||
| ): | ||
| logger.info("PerformanceBiasDetector: permutation test") |
|
|
||
| # if the slice size is too small, use Fisher's exact test, otherwise use a chi-square test | ||
| if min(min(row) for row in ctable) <= max_size_fisher: | ||
| logger.info("PerformanceBiasDetector: Fisher's exact test") |
|
|
||
| ctable = [[slice_x_cnt, slice_y_cnt], [comp_x_cnt, comp_y_cnt]] | ||
|
|
||
| # if the slice size is too small, use Fisher's exact test, otherwise use a chi-square test |
| _, pvalue = scipy.stats.ttest_ind( | ||
| slice_metric.raw_values, comp_metric.raw_values, equal_var=False, alternative=alternative | ||
| ) | ||
| elif metric.name.lower() in ["accuracy", "precision", "recall"]: |
There was a problem hiding this comment.
We should do this check differently. Let’s use some attribute on the metric object, like is_binary_metric or similar.
There was a problem hiding this comment.
maybe it's even better to let the Metric class the responsibility of calculating the contingency table entries
| ) | ||
| except ValueError as err: | ||
| pvalue = np.nan | ||
| logger.info(f"PerformanceBiasDetector: p-value could not be calculated: {err}") |
| value: float | ||
| affected_samples: int | ||
| raw_values: Optional[np.ndarray] = None | ||
| ctable_values: Optional[list[list[int]]] = None |
There was a problem hiding this comment.
We can't have a contingency table
There was a problem hiding this comment.
We can be more general and maybe just get a categorical or binary representation?
Something like binary_counts or binary_representation?
| class Accuracy(ClassificationPerformanceMetric): | ||
| name = "Accuracy" | ||
| greater_is_better = True | ||
| has_contingency_table = True |
There was a problem hiding this comment.
Same as above, this could be more general. In any case, I’m not sure it's needed if we have it on the result object.
There was a problem hiding this comment.
It's not needed but I added it for clarity and "just in case". Should I remove it?
There was a problem hiding this comment.
Actually, it's needed because it's used to decide whether to calculate the binary counts (to prevent from calculating them for classification metrics where it wouldn't make sense, e.g. the F1-Score)
| # column_types=dataset.column_types.copy(), | ||
| # validation=False, |
There was a problem hiding this comment.
Forgot to remove it
| # column_types=dataset.column_types.copy(), | ||
| # validation=False, |
|
|
||
| def test_calculate_slice_metrics(): | ||
| SLICE_SIZE = 500 | ||
| np.random.seed(42) |
There was a problem hiding this comment.
should have the seed somewhere in the detector, not setting the global seed here
There was a problem hiding this comment.
Yes, not sure why I put it there
mattbit
left a comment
mattbit
left a comment
There was a problem hiding this comment.
Looks good, I’ll do some local testing.
| class BalancedAccuracy(ClassificationPerformanceMetric): | ||
| name = "Balanced Accuracy" | ||
| greater_is_better = True | ||
| has_binary_counts = False |
| class F1Score(SklearnClassificationScoreMixin, ClassificationPerformanceMetric): | ||
| name = "F1 Score" | ||
| greater_is_better = True | ||
| has_binary_counts = False |
| class AUC(PerformanceMetric): | ||
| name = "ROC AUC" | ||
| greater_is_better = True | ||
| has_binary_counts = False |
|
Added Fisher's exact test and permutation test for slice metrics significance testing. Todo: unit tests.