Skip to content

Mark only the T case as xfail in the logscore distribution test - #437

Merged
alejandroschuler merged 3 commits into
masterfrom
fix/test-distns-xfail
Nov 10, 2026
Merged

alejandroschuler merged 3 commits into
masterfrom
fix/test-distns-xfail

Conversation

@alejandroschuler

Copy link
Copy Markdown
Collaborator

Found while assessing #168 (PR #433).

test_dists_runs_on_examples_logscore in tests/test_distns.py had this marker:

@pytest.mark.xfail(
    condition=is_t_distribution, reason="Known to fail with T distribution"
)

is_t_distribution is a function, and pytest reads a non-string condition as a truth value. A function object is always true. So the test was marked xfail for every distribution and every learner, not for T alone. A real failure in Normal, Gamma, Weibull or any other case would have shown as xfailed and passed CI. With --slow, the 22 passing cases showed up as xpassed, which is the visible sign of the problem.

Change

  • The T case in the dist parametrize list is now pytest.param(T, marks=pytest.mark.xfail(...)). The marker on the test function and the is_t_distribution helper are gone.
  • The reason names the failure and cites Issue with T distribution #275. NGBRegressor(Dist=T) with default settings raises ValueError: Input y contains NaN from fit, because the natural gradient for log df runs away as df grows. np.exp(params[2]) overflows, and the NaN reaches the base learner.
  • The marker uses strict=True, because T fails for both learners. If T starts to pass, the run fails and the marker has to be removed. I checked that the failure is the same on Python 3.9, 3.12 and 3.14 (the ends and the middle of the CI matrix), so a strict marker should not flip on one CI job.
  • The marker uses raises=ValueError. A different exception from the T case now counts as a real failure.

I checked the rest of tests/ for the same pattern. This was the only xfail. The skipif conditions in test_helpers.py and test_pickling.py call their helper (not _sklearn_has_missing_go_to_left()), so they are real booleans.

Tests

All runs use Python 3.12 and scikit-learn 1.6 with --slow.

tests/test_distns.py::test_dists_runs_on_examples_logscore, 24 cases (12 distributions, 2 learners):

Case Before After
T, both learners xfailed xfailed (strict, raises=ValueError)
the 11 other distributions, both learners (22 cases) xpassed passed

No case fails after the change. No distribution other than T was hiding a failure.

Full suite:

Before After
passed 168 190
xfailed 2 2
xpassed 22 0
skipped 1 1
failed 0 0

The before counts come from two runs on an unmodified export of origin/master: tests/test_distns.py alone (28 passed, 2 xfailed, 22 xpassed) and the rest of the suite (140 passed, 1 skipped). The after count is one run of the whole suite.

Lint on tests/test_distns.py is clean: black --check (24.10.0), isort --check-only, and pylint with the options from .pre-commit-config.yaml (10.00/10). flake8 in that config only covers ngboost/.

🤖 Generated with Claude Code

test_dists_runs_on_examples_logscore passed a function object as the
condition of its xfail marker. pytest treats a non-string condition as a
truth value, and a function is always true, so every distribution and
learner in the test was marked xfail. A real failure in any of them would
have shown as xfailed, and the passing cases showed as xpassed with --slow.

The xfail now sits on the T case alone, through pytest.param. The marker is
strict and only accepts ValueError, because NGBRegressor(Dist=T) with default
settings fails for both learners with "Input y contains NaN" (the natural
gradient for log df runs away as df grows, see #275). The unused
is_t_distribution helper is removed.

Found while assessing #168.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
alejandroschuler and others added 2 commits November 9, 2026 21:34
#434 replaces 'Input y contains NaN' with its own 'The fit diverged'
ValueError. The xfail reason now holds before and after #434 lands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bring in the CI workflow without Python 3.9 (#461) and with one run per
commit (#444). The old workflow on this branch failed on push because
Python 3.9 is not available on the Ubuntu 26.04 runners.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alejandroschuler
alejandroschuler merged commit 8ced9eb into master Nov 10, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants