Re-establish audit fixes (B-001/B-002/M-003/N-001, v0.8.0) against main#13
Closed
settylab-dotto-bot[bot] wants to merge 5 commits into
Closed
Re-establish audit fixes (B-001/B-002/M-003/N-001, v0.8.0) against main#13settylab-dotto-bot[bot] wants to merge 5 commits into
settylab-dotto-bot[bot] wants to merge 5 commits into
Conversation
Three statistical corrections + one default-value harmonization;
all called out in the manuscript-vs-implementation review.
* Mahalanobis denominator sums the two posterior covariances
(cov1 + cov2) instead of averaging them. The manuscript defines
D(a,b) = sqrt((mu_a - mu_b)^T (Sigma_a + Sigma_b)^(-1) (mu_a - mu_b)),
i.e. the variance of the difference of two independent posterior
estimators. The GP-only branch in differential_expression.py was
computing (cov1 + cov2) / 2 even though the sample-variance branch
at the same call site was already summing.
Effect: D shrinks by sqrt(2) in the GP-only regime. Relative
rankings unchanged; FDR re-calibrates against the null. The
resource-estimation comment is brought in line too.
* Differential-abundance posterior tail probability is one-sided:
PTP = Phi(-|z|), matching the manuscript prose ("probability that
the true density difference has the opposite sign to the estimated
change") and the one-sided formula. The implementation was
emitting 2 * Phi(-|z|) via a spurious + np.log(2).
Effect: PTPs are halved; neg_log10_fold_change_ptp increases by
log10(2). A previously-published cutoff of PTP < 1e-3 corresponded
to |z| >= 3.29 (two-sided) and now corresponds to |z| >= 3.09
(one-sided). Re-tune any ptp_threshold chosen against 0.7.0.
* use_empirical_variance defaults to False everywhere. The
recommended kompot.de() path already defaulted to False via
GPSettings; the deprecated wrappers (compute_differential_expression,
compute_smoothed_expression), DifferentialExpression.__init__,
ExpressionModel.__init__, smooth_expression(gp=None), and
smooth_config_template.yaml all defaulted to True. Now consistent
with the manuscript's "empirical variance is disabled by default"
statement. Opt in via use_empirical_variance=True / GPSettings.
Regression coverage in tests/test_audit_fixes.py:
* monkeypatches compute_mahalanobis_distances to capture the
combined-covariance kwarg from a controlled-kernel fit and
asserts equality with cov1 + cov2 (not the average);
* replicates the ln_ptp formula and asserts equality with the
closed-form Phi(-|z|) both directly and end-to-end through
DifferentialAbundance.fit().predict();
* introspects every public entry point's signature and asserts
the default is False; also parses both CLI YAML templates.
tests/test_empirical_variance.py::test_default_is_on was the only
pre-existing test pinning the old default; it has been flipped and
renamed test_default_is_off.
pyproject.toml and kompot/version.py bumped to 0.8.0; CHANGELOG
entry under [0.8.0] - 2026-05-28 documents the migration notes
above. Release tag is not cut by this commit.
align Mahalanobis and DA PTP with manuscript; harmonize use_empirical_variance default; bump to 0.8.0
Revert: audit-fix merge (#11) — premature; code stays on dominik/audit-fixes-2026-05-28
Three statistical corrections + one default-value harmonization;
all called out in the manuscript-vs-implementation review.
* Mahalanobis denominator sums the two posterior covariances
(cov1 + cov2) instead of averaging them. The manuscript defines
D(a,b) = sqrt((mu_a - mu_b)^T (Sigma_a + Sigma_b)^(-1) (mu_a - mu_b)),
i.e. the variance of the difference of two independent posterior
estimators. The GP-only branch in differential_expression.py was
computing (cov1 + cov2) / 2 even though the sample-variance branch
at the same call site was already summing.
Effect: D shrinks by sqrt(2) in the GP-only regime. Relative
rankings unchanged; FDR re-calibrates against the null. The
resource-estimation comment is brought in line too.
* Differential-abundance posterior tail probability is one-sided:
PTP = Phi(-|z|), matching the manuscript prose ("probability that
the true density difference has the opposite sign to the estimated
change") and the one-sided formula. The implementation was
emitting 2 * Phi(-|z|) via a spurious + np.log(2).
Effect: PTPs are halved; neg_log10_fold_change_ptp increases by
log10(2). A previously-published cutoff of PTP < 1e-3 corresponded
to |z| >= 3.29 (two-sided) and now corresponds to |z| >= 3.09
(one-sided). Re-tune any ptp_threshold chosen against 0.7.0.
* use_empirical_variance defaults to False everywhere. The
recommended kompot.de() path already defaulted to False via
GPSettings; the deprecated wrappers (compute_differential_expression,
compute_smoothed_expression), DifferentialExpression.__init__,
ExpressionModel.__init__, smooth_expression(gp=None), and
smooth_config_template.yaml all defaulted to True. Now consistent
with the manuscript's "empirical variance is disabled by default"
statement. Opt in via use_empirical_variance=True / GPSettings.
Regression coverage in tests/test_audit_fixes.py:
* monkeypatches compute_mahalanobis_distances to capture the
combined-covariance kwarg from a controlled-kernel fit and
asserts equality with cov1 + cov2 (not the average);
* replicates the ln_ptp formula and asserts equality with the
closed-form Phi(-|z|) both directly and end-to-end through
DifferentialAbundance.fit().predict();
* introspects every public entry point's signature and asserts
the default is False; also parses both CLI YAML templates.
tests/test_empirical_variance.py::test_default_is_on was the only
pre-existing test pinning the old default; it has been flipped and
renamed test_default_is_off.
pyproject.toml and kompot/version.py bumped to 0.8.0; CHANGELOG
entry under [0.8.0] - 2026-05-28 documents the migration notes
above. Release tag is not cut by this commit.
katosh
force-pushed
the
main
branch
2 times, most recently
from
096fa2a to
62e83b6
Compare
Contributor
Author
|
Superseded by #14.
The branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-establish audit fixes against
mainThis PR re-proposes the audit-fix changeset that was prematurely merged via
#11and then reverted via
#12to restoremainto its pre-#11state (v0.7.0).The fixes are now offered cleanly against
mainfor proper review.Why a fresh branch
#11was reverted with agit revert -m 1merge (#12). Because the originalaudit-fix commit is already an ancestor of
mainthrough the reverted mergecommit, a PR from the original head branch (
dominik/audit-fixes-2026-05-28)shows an empty diff ("already merged") even though the revert undid its
effect — the classic reverting-a-merge caveat. To produce a clean, reviewable
diff, the audit-fix commit was cherry-picked onto the post-revert
maininto anew branch,
dominik/audit-fixes-2026-05-28-rebased. The original head branch isleft intact. Diff scope is identical to the original changeset (12 files,
+314/-18).
What this restores
use_empirical_variance=Falsedefault behavior.0.8.0.tests/test_audit_fixes.py;CHANGELOG.md,pyproject.toml, andkompot/version.pyupdated accordingly.Caveat
Should
#11's merge ever be re-merged later, re-merging the reverted commitwould not re-apply these changes (same merge-revert caveat) — this fresh branch
sidesteps that.
Left open for review.