Add unit tests for xrex/models/loss_recsys.py - #75
Open
AnonRish wants to merge 1 commit into
Open
Conversation
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
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.
Summary
multihot_loss_compute,continuous_loss_compute,and
tweedie_loss_computedevdependency group (pytest) to pyproject.toml, since nothingin 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
Notes for reviewers
multihot_loss_compute's tests need an explicit single-device JAX mesh andNamedSharding-wrapped inputs (see the
_shard()helper) because of itsinternal
with_sharding_constraintcall -- this isn't test scaffoldingoverhead 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_weightsin the same function must beshaped like
padding_mask((batch, seq)), not a flat per-row array -- ashape 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.