[Logiquo]

Summary

Adds processors for per-step labels, and normalization and missing-value options for time series and tensor features.

Label processors

timeseries_binary and timeseries_regression (TimeseriesLabelProcessor) take a label time series (timestamps, values), place it on the same uniform time grid as TimeseriesProcessor (with the same first timestamp and sampling_rate as the input, each label lines up with one input step), and return (value, mask), with mask 0 for steps without a label. Missing labels are masked, never imputed. Their registered names match the timeseries_binary / timeseries_regression model modes, so models predict and are scored at every step. The gridding is shared by both processors through sample_on_grid.

output_schema = {"los": "timeseries_regression"}
# sample["los"] = (timestamps, values)

Normalization and missing-value options

All options are off by default, so existing behaviour is unchanged.

• TimeseriesProcessor:
  • normalization: "zscore" or "minmax", learned in fit() from the observed (non-missing) values and applied before imputation. With "zscore" and the existing impute_strategy="forward_fill", values before the first observation become 0, which is the training mean.
  • missing_indicators: appends one 0/1 indicator per feature, 1 where the value was missing before imputation.
• TensorProcessor: the same normalization and missing_indicators, plus impute_strategy="zero".

Both processors set learns_statistics when normalizing, so PyHealth warns when they are fitted on all samples instead of a training split (set_task(task, split=...)). The shared normalization helpers live in the new pyhealth/processors/utils.py.

input_schema = {
    "vitals": ("timeseries", {"normalization": "zscore", "missing_indicators": True}),
    "demographics": ("tensor", {"normalization": "zscore", "impute_strategy": "zero"}),
}

Tests

• tests/core/test_per_step_labels.py: the label processors (masks, grid, binary validation) and end to end with RNN and the Trainer.
• tests/core/test_processor_normalization.py: the statistics (streamed across samples, matching NumPy), constant and unobserved features, unchanged defaults, and the processor options.

Existing processor tests pass.

[github-actions]

Test Coverage

Total: 72.43% (19304 / 26653 lines)

Modified file	Coverage	Lines
pyhealth/processors/__init__.py	94.12%	32 / 34
pyhealth/processors/temporal_timeseries_processor.py	78.57%	44 / 56
pyhealth/processors/tensor_processor.py	92.50%	74 / 80
pyhealth/processors/timeseries_processor.py	84.48%	98 / 116
pyhealth/processors/utils.py	96.55%	56 / 58

Workflow run · 64b7cca

[jhnwu3]

lgtm

[jhnwu3]

Sorry will undo this on the draft for now, accidentally sped through it.

[Logiquo]

/review

[github-actions]

Previous review

_{Reviewed commit 227808a}

[github-actions]

Previous review

AI Review

suggestion: need change
decision: need change

Thank you for this contribution. It adds per-step label processors (timeseries_binary and timeseries_regression) that return a value and a mask. It also adds opt-in normalization and missing-value indicators for TimeseriesProcessor and TensorProcessor, with shared statistics in processors/utils.py. Defaults stay unchanged, and the tests are thorough. Coverage meets the bar: the changed files are at 84–95% and the whole repository is at 71.9%.

It is close. A few things need fixing first.

To get this merged:

1. Reuse the gridding helper. sample_on_grid in timeseries_processor.py is nearly a verbatim copy of the gridding loop in TemporalTimeseriesProcessor (temporal_timeseries_processor.py, around lines 96–106). Please have that processor call the new helper, or reuse its existing logic, so the grid logic lives in one place.
2. Make the label docs match the behavior. The TimeseriesLabelProcessor docstring says labels "not on the grid" are masked. In practice sample_on_grid floors each timestamp into a step. When several labels fall in the same step, the last one silently overwrites the others. Please either document this clearly or raise or mask on collisions. Masking them out would fit the "never imputed" intent.

Optional suggestions:

• Check or document that the label's first timestamp must equal the input's first timestamp for the steps to line up. A label series longer than its input currently fails only inside the loss during training.
• With normalization on, a scalar TensorProcessor input comes out with shape (1,) while dim() reports 0. Please make these consistent.
• Add the new public classes to docs/api/processors.rst.
• tests/core/test_per_step_labels.py seems to repeat several model-level checks already in tests/core/test_timeseries_modes.py. You could trim them.

_{Reviewed commit 227808a}

[Logiquo]

/review

[github-actions]

_{Reviewed commit 64b7cca}

[github-actions]

[low] grid_steps truncates toward zero instead of flooring

The docstring says a timestamp goes to floor((t - t0) / sampling_rate), but int(...) truncates toward zero. A timestamp earlier than the first one (only possible with unsorted input) such as t0 - 30 min maps to step 0 rather than being dropped, so it can silently overwrite or be placed on step 0. The same truncation existed in the old TimeseriesProcessor code, but the new label processor relies on this helper for its duplicate check. Fix: use int(np.floor((t - start_time) / sampling_rate)) or math.floor, or document that timestamps must be sorted.

[github-actions]

[low] NaN labels count toward the same-step duplicate check

The duplicate check runs over all timestamps, including those whose label is NaN. The class docstring says NaN labels are masked as missing, yet a NaN sharing a step with a real label raises "2 labels fall into time step ..." instead of being ignored. Fix: compute grid_steps only over the entries where labels is not NaN, for example grid_steps([t for t, v in zip(timestamps, labels) if not np.isnan(v)], ...), with the same handling for scalar NaN and length-check edge cases.

[github-actions]

[low] Label/input alignment is a caller contract that is not validated

The processor cannot see the input series, so labels only line up if the caller gives them the same first timestamp and sampling_rate. The docstring states this. However, base_model._masked_step_loss silently truncates a label that is shorter than the input, so a label series starting later is shifted without any error; only longer labels are rejected. Non-blocking because the contract is documented. A fix would be an optional start time argument, or a check in the task that builds the labels.

[github-actions]

[low] Minmax leading-gap fill is not documented

With forward_fill, values before the first observation are set to 0 before the fill. That is the training mean for zscore, but for minmax it is the training minimum, since the center is low. The docstring documents only the zscore case. Fix: state the minmax meaning, or fill with a neutral value for minmax.

[github-actions]

[low] Test named for sampling_rate does not pass a sampling rate

test_grid_follows_sampling_rate uses the default one-hour rate, so no test covers a non-default sampling_rate for the label processors. Pass e.g. sampling_rate=timedelta(hours=2) and check the step placement, so the name matches the test.

[github-actions]

AI Review

suggestion: approve
decision: approve

Thank you for this contribution. It adds per-step label processors (timeseries_binary and timeseries_regression) that return (value, mask) on the input's time grid. It also adds opt-in normalization and missing_indicators options for TimeseriesProcessor and TensorProcessor, plus impute_strategy="zero" for TensorProcessor, and a shared utils.py. The defaults are unchanged and the new options are off unless requested. The grid logic is consolidated into sample_on_grid rather than duplicated. The tests cover the statistics against NumPy, the masks and grid, the unchanged defaults, and an end-to-end run with RNN and Trainer. I think it looks ready to merge.

Coverage: the whole repository is at 72.43%, above the 60% bar. Most changed files are above 80%: utils.py 96.6%, __init__.py 94.1%, tensor_processor.py 92.5% and timeseries_processor.py 84.5%. temporal_timeseries_processor.py is at 78.6%, slightly under 80%. Your change there is a small refactor onto sample_on_grid that is exercised by a test, so I don't see this as blocking.

Optional suggestions:

• grid_steps uses int(), which truncates toward zero. Timestamps earlier than the first one would land on step 0, which doesn't match the "floor" wording in the docstring. Sorted inputs are unaffected.
• A NaN label that shares a step with a real label currently raises the duplicate-step error. You could treat NaN as "no label" and exclude it from that check.
• Add tests for a non-default sampling_rate on the label processors, for minmax at the processor level, and for the learns_statistics warning.
• Note in the docs that minmax with forward_fill maps leading gaps to the training minimum.

_{Reviewed commit 64b7cca}

the original reason for this request change is "Sorry will undo this on the draft for now, accidentally sped through it."