Repository navigation
Towards Unified Sequence Representation IV: Add RNN_V2 for SequenceProcessorV2 fields - #1353
Conversation
Test CoverageTotal: 75.14% (21431 / 28522 lines)
|
a471c25 to
22f972c
Compare
22f972c to
2e7d1fa
Compare
2e7d1fa to
8e64f6f
Compare
8e64f6f to
afe957d
Compare
afe957d to
65addd0
Compare
65addd0 to
fb711f5
Compare
Logiquo
changed the title
1cba453 to
92cd451
Compare
Unified_RNN embeds each sequence_v2 field with SequenceEmbeddingModel and
encodes it bottom-up, level by level: a set level mean-pools each group,
a sequence or series level runs an RNN over each group and keeps the last
output. A group without records is skipped and summarized as zeros.
Static tensor fields are used directly or through a projection; the
summaries and statics are concatenated and a linear layer predicts the
label.
- Per-field settings (hidden_dim, rnn_type, num_layers, dropout,
bidirectional, time_feature) take one value or a {field: value} dict.
- time_feature adds the time since the previous record of the group on
timed levels.
- Per-step labels: one prediction per label record, from each field's
top-level state after its last record at or before the label's time,
so a prediction never sees later records. Fields need a series top
level and the RNNs must be causal.
- RNN and MultimodalRNN now point to Unified_RNN for per-step labels.
5c8539d to
31cfc46
Compare
Logiquo
added
the
status: automatic
|
/reiview |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Previous review
AI Review
suggestion: need change
decision: need change
Thank you for this contribution. RNN_V2 is a useful first model for SequenceProcessorV2 fields. It encodes each field bottom-up, supports static tensors, and handles per-step labels. The tests are thorough, and coverage meets the bar: rnn.py is at 93.8%, every other changed file is above 88%, and the repository total is 75.1%.
To get this merged:
- The per-step path only requires a
seriestop level, but a timed level below the top can still leak the future. For example, take labs nested under a timed stay or visit. All of that stay's lab records are folded into the top-level state, whatever their own times. So a label at time t can see child records after t. This contradicts the documented guarantee that a prediction never sees later records. Please either reject per-step labels when any lower level is timed (or otherwise cannot be proven causal), or make the lower levels respect the label time. Please also add a test with a nested timed level that checks the model rejects it or does not leak.
Optional suggestions:
- The YAIB test does a single forward pass. Running a short training step would match the "end to end" wording in the description.
- Add numeric checks for the RNN and LSTM types, bidirectional in the non-per-step case, and
time_feature. Right now they are only checked for output shape.
Reviewed commit 31cfc46
…children SequenceProcessorV2 levels each have their own time unit and origin, and a record forms one unit with its children; times are only compared within a level. Records, Resample and LevelConfig said "the task's time unit", which suggested a shared timeline. RNN_V2's per-step guarantee is now stated for top-level records: a label uses each top-level record at or before it with everything it contains, and lower levels' times are not compared to labels. A test pins this down.
|
/review |
| fields' top level, with the same origin, and a label at time ``t`` uses | ||
| each field's top-level state after its last top-level record with time | ||
| ``<= t`` (zeros before the first) and the statics, so it never sees a | ||
| later top-level record. That state covers everything the record contains: |
There was a problem hiding this comment.
[medium] Per-step predictions see later child records of a top-level record
For per-step labels, the top-level state is taken from the last top-level record with time <= label time, but that record's children are encoded whole, with no cut at the label time. A label at t therefore sees timed children whose times are > t. The new test test_a_top_level_record_includes_all_its_children (tests/core/test_rnn_v2.py) asserts exactly this: a label at 1.0 uses a stay at 0.0 that contains labs at 5.0 and 9.0. This contradicts the PR description's "A prediction never sees later records". Fix either by excluding lower-level records with time > label time before pooling or encoding, or by rejecting per-step models whose lower levels carry time, and then state the limit in the description.
| torch.testing.assert_close(values, old[:, :4]) | ||
| torch.testing.assert_close(mask.float(), old[:, 4:]) | ||
|
|
||
| def test_tasks_train_with_rnn_v2(self): |
There was a problem hiding this comment.
[low] Test named 'train' only runs a forward pass
test_tasks_train_with_rnn_v2 builds the model and checks one forward pass (loss finite, logit count). It never calls Trainer.train or backward. Either rename it to reflect the forward check, or train one epoch to match its name.
| with self.subTest(top=top), self.assertRaisesRegex(ValueError, "Wrap"): | ||
| RNN_V2(dataset, embedding_dim={"x": [{"tokens": (4,)}, {}]}) | ||
|
|
||
| def test_no_bidirectional(self): |
There was a problem hiding this comment.
[low] Non-per-step bidirectional RNN_V2 path has no test
The bidirectional path (rnn.py _FieldRNN / RNNLayer with bidirectional=True) is only exercised for rejection with per-step labels. No test builds a bidirectional RNN_V2 on a non-per-step label and checks output shapes. Add a small forward-shape test for bidirectional=True on a binary label.
There was a problem hiding this comment.
AI Review
suggestion: approve
decision: approve
Thank you for this contribution. RNN_V2 is a useful addition: it encodes sequence_v2 fields bottom-up, handles per-step labels, and ties into the existing per-step tests and the YAIB tasks.
It looks ready to merge. The implementation matches your description, and the tests are thorough. They include a loop-reference check, a test that predictions don't see later records, empty groups, resampled top levels, and the YAIB tasks run end to end.
Coverage: rnn.py is at 93.8% and the other changed files are between 88% and 100%. The repository total is 75.1%. That meets the bar of 80% per file and 60% overall.
Optional suggestions:
- Causality wording: Per-step predictions use the whole top-level record, including child records whose times are later than the label. The docstring says this and a test covers it, so I don't treat it as blocking. The summary line "A prediction never sees later records" could say "later top-level records" so it isn't read as a stronger guarantee.
- YAIB test name: test_tasks_train_with_rnn_v2 only runs a forward pass, not training. Either rename it or add a short Trainer step.
- Missing test: Add a test for the non-per-step bidirectional=True path.
- Formatting: There is a stray double blank line before test_lstm in test_rnn_v2.py.
Reviewed commit 89c68c0
Fourth PR in the stack toward a unified sequence representation, on top of #1352. It adds
RNN_V2, the first model that consumesSequenceProcessorV2fields and per-step labels.Changes
sequence_v2field is embedded bySequenceEmbeddingModeland encoded bottom-up, level by level. Asetlevel mean-pools each group; asequenceorserieslevel runs an RNN over each group and keeps the last output. A group without records is skipped and summarized as zeros.tensorfields are used directly or through a projection. The field summaries and statics are concatenated, and a linear layer predicts the label. Other processors are rejected; sequences, sets and tokens are expressed withsequence_v2.hidden_dim,rnn_type,num_layers,dropout,bidirectionalandtime_featuretake one value, or a{field: value}dict.time_featureadds the time since the previous record of the group on timed levels.seriestop level, and bidirectional RNNs are rejected.RNNandMultimodalRNNnow point toRNN_V2for per-step labels.RNN_V2, and the YAIB tasks are tested end to end with it.