-
Notifications
You must be signed in to change notification settings - Fork 555
TCN Bounty Implementation #729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR successfully updates the TCN (Temporal Convolutional Networks) model to the current PyHealth 2.0 API, replacing the deprecated SampleEHRDataset with SampleDataset and adopting the EmbeddingModel architecture. The implementation provides dilated causal convolutions for temporal sequence modeling on EHR data, supporting both discrete code sequences (diagnoses, procedures) and continuous timeseries data.
Key changes:
- Modernized API: Updated from legacy
SampleEHRDatasettoSampleDatasetwithEmbeddingModel - Simplified architecture: Removed complex manual feature processing in favor of
EmbeddingModel's automatic handling - Improved PyTorch compatibility: Updated
weight_normimport to PyTorch 2.0+ path (torch.nn.utils.parametrizations)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pyhealth/models/tcn.py |
Complete TCN model refactor to PyHealth 2.0 API; removed 150+ lines of legacy code; updated weight_norm import; added safety check in Chomp1d; improved forward pass with explicit last-output extraction |
tests/core/test_tcn.py |
New comprehensive test suite with 6 test cases covering initialization, forward/backward passes, embeddings, and custom hyperparameters |
examples/tcn_mimic3_codes.ipynb |
New example notebook demonstrating mortality prediction on MIMIC-III dataset with clear step-by-step instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Get the actual output dimension from TCNLayer instances | ||
| # All TCNLayers have the same output dimension | ||
| self.num_channels = next(iter(self.tcn.values())).num_channels |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential StopIteration error if self.feature_keys is empty. Consider adding a check:
if not self.feature_keys:
raise ValueError("At least one input feature is required")before line 240, or handle the case where self.tcn might be empty.
| Examples: | ||
| >>> from pyhealth.datasets import SampleEHRDataset | ||
| >>> samples = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add usage examples in the new docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a usage example in the docstring. I also changed SampleDataset to create_sample_dataset in the test file.
42b3223 to
ffc94bb
Compare
Bilal Arif
netID: barif
Contribution Type: Updated model
Description:
Files to review: