Skip to content

Add unit tests for xrex/models/loss_recsys.py - #75

Open
AnonRish wants to merge 1 commit into
xai-org:mainfrom
AnonRish:add-xrex-loss-tests
Open

Add unit tests for xrex/models/loss_recsys.py#75
AnonRish wants to merge 1 commit into
xai-org:mainfrom
AnonRish:add-xrex-loss-tests

Conversation

@AnonRish

Copy link
Copy Markdown

Summary

  • Add 17 tests covering multihot_loss_compute, continuous_loss_compute,
    and tweedie_loss_compute
  • Add a dev dependency group (pytest) to pyproject.toml, since nothing
    in the project currently declares a test runner

Problem

phoenix/xrex has 165 Python files and, as far as I could find, zero test
coverage (no tests/ directory anywhere under phoenix/, and grepping for
pytest/unittest usage turns up one incidental hit). loss_recsys.py --
the multihot, continuous (mse/mae/huber), and Tweedie loss functions used
in training -- is pure and deterministic, so it's a good place to start.

Coverage

Correctness on hand-checkable inputs (perfect predictions -> near-zero
loss); masking behavior (padding, negative-sample exclusion); weighting;
clamping and flooring of inputs; all three loss_type branches of
continuous_loss_compute; all three p-value branches of the Tweedie
deviance, checked against the closed-form Poisson and Gamma special cases
at p=1 and p=2; error handling for an unknown loss_type.

Verification

uv sync --group dev
uv run pytest xrex/models/test_loss_recsys.py -v
# 17 passed

Notes for reviewers

multihot_loss_compute's tests need an explicit single-device JAX mesh and
NamedSharding-wrapped inputs (see the _shard() helper) because of its
internal with_sharding_constraint call -- this isn't test scaffolding
overhead added for style, it's what the function actually requires even
for a single-array, single-device call. Flagging in case that's not
intentional/known. Separately, raw_weights in the same function must be
shaped like padding_mask ((batch, seq)), not a flat per-row array -- a
shape mismatch broadcasts silently into something numerically meaningless
instead of raising. Not fixed here (out of scope for a test-only PR);
worth a one-line docstring note if someone sends a follow-up.

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.

1 participant