[Logiquo]

Third PR in the stack toward a unified sequence representation, on top of #1347. It adds SequenceEmbeddingModel, the embedding building block for models over SequenceProcessorV2 fields.

Changes

• SequenceEmbeddingModel embeds the packed records of every sequence_v2 field of a batch, level by level. A record's embedding is one nn.Embedding per token namespace (<pad> embeds to zeros) and a linear projection of its values, concatenated. Offsets and times are passed through, so models can encode the hierarchy bottom-up.
• embedding_dim takes one LevelEmbedding per level and field.
• flatten_spatial keeps spatial value axes (e.g. ECG samples) apart and projects at each position.
• pretrain initializes token namespaces from text-vector files (PretrainedEmbedding).
• mask="concat" projects a level's values together with its missing indicators.
• Exported from pyhealth.models, with an API page.

[github-actions]

Test Coverage

Total: 74.97% (21247 / 28339 lines)

Modified file	Coverage	Lines
pyhealth/models/__init__.py	100.00%	53 / 53
pyhealth/models/sequence_embedding.py	100.00%	148 / 148

Workflow run · cbf1df2

[Logiquo]

/review

[github-actions]

_{Reviewed commit cbf1df2}

[github-actions]

[low] Reserved pretrain 'normalize' option is an unimplemented stub

PretrainedEmbedding exposes normalize (TypedDict key at line 68, documented at line 52 as 'Reserved for future work; only None'), but the only behaviour is to raise NotImplementedError when it is set. That adds a public option that cannot be used and that the docs themselves call future work. Drop the key and the check, or leave it out until it is implemented. Non-blocking.

[github-actions]

[low] Pretrained vectors of the wrong size are loaded silently

init_embedding_with_pretrained returns the number of tokens it loaded, and this call discards it. A file whose vectors are narrower than the namespace's embedding size loads nothing, and a wider one is silently truncated, with no error or warning. The PretrainedEmbedding docstring says the vector size must match, but nothing enforces it. Consider checking the returned count (for example, raising or warning when it is 0), or checking the vector width against embedding.embedding_dim on the first line. Non-blocking, since the requirement is documented.

[github-actions]

AI Review

suggestion: approve
decision: approve

Thank you for this contribution. SequenceEmbeddingModel gives models a way to embed the packed records of sequence_v2 fields level by level. It supports per-namespace token embeddings, value projections, spatial-preserving projection, mask concat, and pretrained initialization.

The change looks ready to merge. It is self-contained: a new model, an export and an API page. It doesn't alter existing base classes or public behavior. I found no correctness issues in the shape logic. The tests cover configuration errors, offsets and time, pad-to-zero, spatial handling, mask modes, pretraining and a gradient pass through a DataLoader batch. Coverage is 100% for the new sequence_embedding.py and for the modified init.py. Whole-repository coverage is 74.97%, above the 60% bar.

Optional suggestions:

• PretrainedEmbedding.normalize only raises NotImplementedError. Consider dropping it until it is implemented, to keep the API surface small.
• If a pretrain file's vector size doesn't match the embedding size, the vectors may load truncated or as zeros without any warning. Consider checking the size or warning on a mismatch.
• The docstring examples aren't run as doctests, so it's worth checking them by hand.

_{Reviewed commit cbf1df2}