[AxelNoun]

Contributor: AxelNoun (GitHub: @AxelNoun), external contributor (no NetID)
Contribution Type: New Dataset + Task
Description: Adds PTB-XL (12-lead ECG, PhysioNet v1.0.3, open access) as a
PyHealth dataset, with a 5-superclass multi-label diagnosis task and a
split helper using the official stratified folds. Motivated by the CPBench
effort: it gives conformal prediction a multi-label ECG benchmark that needs
no credentialing, with metadata suited to covariate/label-shift splits.
Based on Wagner et al., Scientific Data 2020
(https://www.nature.com/articles/s41597-020-0495-6).

Relation to CardiologyDataset: master lists ptb-xl among the six
subsets of CardiologyDataset, which reads the PhysioNet/CinC Challenge 2020
copy (.mat/.hea files under ptb-xl/g*/, one patient id per g* folder)
for the binary cardiology_is*_fn tasks. It subclasses the 1.x
BaseSignalDataset, now a deprecation stub, so an instance on current
master has no set_task or stat. PTBXLDataset reads the PTB-XL v1.0.3
release itself on the BaseDataset/YAML API and exposes what that path does
not: PTB-XL patient ids, strat_fold, SCP statements with likelihoods,
site/device, and a choice of 100 or 500 Hz records. (The cardiology_is*_fn
tasks read only age, sex and Dx codes from the .hea headers and assume
500 Hz.)

Output label order: MultiLabelProcessor.fit sorts observed labels
alphabetically. On the full v1.0.3 corpus the multi-hot vector is
CD, HYP, MI, NORM, STTC. The task does not pin a custom vocabulary.

Files to Review:

• pyhealth/datasets/ptbxl.py: dataset implementation
• pyhealth/datasets/configs/ptbxl.yaml: table config (v1.0.3 template)
• pyhealth/tasks/ptbxl.py: 5-superclass multi-label task
• pyhealth/datasets/splitter.py: split_by_strat_fold helper
• tests/core/test_ptbxl.py: tests with synthetic fixtures
• docs/api/datasets/pyhealth.datasets.PTBXLDataset.rst, docs/api/tasks/pyhealth.tasks.ptbxl.rst
• examples/ecg/ptbxl/ptbxl_superclass_quickstart.py

Implementation notes:

• wfdb is an optional extra (pip install 'pyhealth[ptbxl]'), lazily imported.
• PTBXLDataset.root is the user-provided data path. Derived metadata CSVs
  live under metadata_cache_dir (not root) so read-only mounts stay
  untouched. BaseDataset.load_table concatenates root / file_path and
  cannot take an absolute path, so load_table is overridden to scan the
  cache directory. Cache identity hashes the data-root path and the derived
  CSV bytes into dataset_name (EEGBCI pattern) so two roots or an in-place
  ptbxl_database.csv replace cannot share global_event_df.
• Cache key after Key the dataset cache on the config and source files #1265: BaseDataset now also hashes the resolved config
  and fingerprints table sources, but it resolves file_path against root,
  where the derived CSV does not live. On the test fixture the fingerprint is
  empty (key["sources"] == {"records": [[]]}), so identity rests on the
  config hash (the derived filename encodes the root and
  ptbxl_database.csv hashes) and on dataset_name. The code comment in
  ptbxl.py now says this.
• PYHEALTH_REQUIRE_CACHE_DIR (Warn about the default dataset cache; allow requiring cache_dir #1264): the derived CSV and the resolved
  YAML are written before super().__init__(). With the variable set and no
  cache_dir, they are written to metadata_cache_dir (default
  ~/.cache/pyhealth/datasets/ptbxl/) and then BaseDataset raises
  (observed on the test fixture). Not changed in this PR.
• Resolved cache YAML: BaseDataset only accepts config_path, so PTB-XL
  writes a rate/root/source-keyed YAML next to the derived CSV before
  super().__init__(). CSV and YAML writes use tmp+os.replace under
  filelock.FileLock.
• Unrelated pre-existing # noqa: E402 on the EEGBCI import line in
  pyhealth/datasets/__init__.py: left untouched (outside the diff).
• Signals returned as (n_leads, n_samples), matching EEGBCI's convention.
• Sample age is always an int: a missing age becomes -1
  (age_is_missing=True), and the HIPAA-censored raw value 300 is clipped to
  90 (age_is_censored=True). The raw 300 remains in the derived metadata
  CSV.
• Empty superclass sets (about 400 records on v1.0.1, Wagner et al. Table 9;
  mainly pacemaker ECGs) are dropped by default via drop_empty_labels.
• split_by_strat_fold(..., folds=) avoids decoding 12-lead signals just to
  read an integer. check_patient_disjoint=True raises if a patient_id
  appears in more than one split (official PTB-XL folds are patient-disjoint;
  the synthetic fixture leaks patient 15709 across folds 1 and 9 to exercise
  this).
• Fixture under test-resources/core/ptbxl/ is synthetic CSVs; WFDB
  waveforms are generated at test time (not committed).

Testing / CI:

• Local, after merging master at e94ca1d (includes Truncate nested inputs longer than the fitted width #1263 to Key the dataset cache on the config and source files #1265 and
  Fit processors on the training split in set_task (split=PatientSplit) #1273), head f4d5d07
  (Windows, Python 3.13.5, wfdb 4.3.1), each file run with
  python -m unittest discover -t tests -s tests/core -p '<file>' -v:
  test_ptbxl.py 28 passed, test_dataset_cache_key.py 4 passed,
  test_default_cache_dir.py 5 passed.
• python tools/check_pr_rules.py --base e94ca1d --head HEAD passed
  (ruff 0.16.10).
• Without wfdb / pyhealth[ptbxl], waveform I/O and set_task e2e tests skip.
• pixi test feature uses pyhealth = { extras = ["ptbxl"] } so make testall under pixi covers waveform I/O.

Follow-up: 71-SCP-code classification and age regression are anticipated
in the structure but not implemented here; they are planned as a separate PR.

Test plan

• python -m unittest discover -t tests -s tests/core -p 'test_ptbxl.py' -v: 28 passed (with wfdb), on f4d5d07
• test_dataset_cache_key.py (4 passed) and test_default_cache_dir.py (5 passed), on f4d5d07
• python tools/check_pr_rules.py --base e94ca1d --head HEAD: passed
• CI make testall / contribution-rules on the PR: green on fe1007f (18 Aug)
• CI make testall / contribution-rules on f4d5d07 (after the master merge)

---

Recreates #1186, which was closed on 12 Aug when the fork hosting its branch
was detached during an account cleanup on my side. This was my error, and I
apologise for the noise. #1186 received no review; its only comment was my note
on the pre-existing # noqa: E402, repeated above.

[AxelNoun]

@jhnwu3 This PR (PTB-XL dataset and 5-superclass task) has not been reviewed yet. I merged current master (e94ca1d) into the branch; locally, test_ptbxl.py (28 tests), test_dataset_cache_key.py (4) and test_default_cache_dir.py (5) pass, as does tools/check_pr_rules.py, and CI on the new head is green (build and contribution-rules).

One question following #1264: with PYHEALTH_REQUIRE_CACHE_DIR=1 and no cache_dir, the derived metadata CSV and the resolved YAML are still written to metadata_cache_dir (default ~/.cache/pyhealth/datasets/ptbxl/) before BaseDataset raises. Should their default location follow cache_dir instead, so that nothing is written outside cache_dir when the variable is set?

Could you review this PR, or indicate who should?

[github-actions]

Test Coverage

Total: 71.70% (18885 / 26338 lines)

Modified file	Coverage	Lines
pyhealth/datasets/__init__.py	83.33%	40 / 48
pyhealth/datasets/ptbxl.py	83.51%	157 / 188
pyhealth/datasets/splitter.py	96.62%	343 / 355
pyhealth/tasks/__init__.py	89.19%	33 / 37
pyhealth/tasks/ptbxl.py	93.42%	71 / 76

Workflow run · 5d874df

[AxelNoun]

I have merged upstream/master (4a173f7) into this branch. build and contribution-rules both pass on the merge commit, and the PR reports mergeable: true again.

The only conflict was in pyproject.toml, where both sides had added an extra at the same place in [project.optional-dependencies]: ptbxl here and xgboost on master. Both are kept.

I also checked that the automatic merge of pyhealth/datasets/splitter.py is coherent rather than merely conflict free, since master reworked that module substantially while this PR adds a function to it. subset, patient_to_index and split_by_patient are all still present, split_by_strat_fold only ever uses those three, and pyhealth/datasets/__init__.py exports both the new Split and PatientSplit and split_by_strat_fold.

This PR has not been reviewed since it was opened on 13 August. Nothing mechanical blocks it now, so a review would be welcome whenever one of you has a slot. @jhnwu3

One request I cannot carry out myself: the status: conflict label is still on this PR although the conflict is resolved. It was applied by hand rather than by a workflow, and I only have read access here, so a maintainer needs to remove it.

[Logiquo]

/review

[github-actions]

Previous review

AI review: findings

Adds PTBXLDataset (BaseDataset + YAML; reads PTB-XL v1.0.3 metadata and writes a derived CSV under metadata_cache_dir), PTBXLSuperclassClassification (5-superclass multi-label, drops empty label sets by default), and split_by_strat_fold, plus docs, an example, a ptbxl wfdb extra, a pixi test extra, and synthetic CSV fixtures. It does not modify any base class; it only adds public exports and an extra. Not redundant: CardiologyDataset and the cardiology_* tasks read the Challenge 2020 copy of PTB-XL for binary labels, a different loader and release, and the new 5-class task has no existing counterpart. Tests cover metadata writing, censored and missing age, label aggregation, the fold splitter, patient-leak detection, and set_task end to end, but the waveform and set_task tests skip without wfdb and nothing exercises the 500 Hz path. Blocking (medium): pyhealth/datasets/ptbxl.py:344 documents sex as 0 = female, 1 = male, which appears inverted relative to PhysioNet's PTB-XL documentation (0 = male); I could not check the release README offline, and the same mapping is repeated in configs/ptbxl.yaml:19 and tasks/ptbxl.py:269, so sex-based subgroup analysis would be silently mislabeled. Blocking (medium): the 500 Hz branch (filename_hr, records500) is untested; tests/core/test_ptbxl.py:219 only checks the derived filename, and no test loads a 250-sample record. Non-blocking (low): unused superclass_names ClassVar, example decodes every waveform through split_by_strat_fold, the v1.0.1/v1.0.2 rejection described in the docstring is not enforced, the output vocabulary depends on the data observed, and PYHEALTH_REQUIRE_CACHE_DIR writes files before failing. Overall risk is moderate-low: both blocking items are fixable without design changes.

Findings outside the diff

• [low] Label vocabulary is data-dependent, not fixed to five classes tasks/ptbxl.py:211
  output_schema is multilabel, so MultiLabelProcessor.fit builds the vocabulary from the labels observed in the dataset it is given. A subset with no CD or STTC records gets a smaller output dimension without any error, even though the task is described as 5-superclass. The docstring states this, so this is non-blocking. Fix if desired: pin the vocabulary to PTBXL_DIAGNOSTIC_SUPERCLASSES through a processor configured with those labels.

_{Reviewed commit 2271703}

[github-actions]

Previous review

AI Review

suggestion: need change
decision: not available

Thank you for this contribution. PTB-XL support with a 5-superclass multi-label task, official fold splits and a cache-aware metadata pipeline is a useful addition, and the PR is well documented and well tested. It is close, but one blocking problem remains, plus one test gap.

To get this merged:

1. Fix the sex encoding. The code, YAML and docs state sex: 0 = female, 1 = male. As I recall PhysioNet's PTB-XL documentation, the coding is the opposite: 0 = male, 1 = female. Please check this against the v1.0.3 release README. If it is inverted, fix every place the mapping is stated: the comment at pyhealth/datasets/configs/ptbxl.yaml:19, the PTBXLDataset docstring note, the comment in pyhealth/tasks/ptbxl.py, and the fixture README if it mentions it. The integer values pass through unchanged, so this is only a labelling fix. Left as is, it would silently flip sex-based subgroup and shift analyses.
2. Add a test for the 500 Hz path. The 500 Hz fixtures are generated at test time, but nothing loads one. test_100_and_500_hz_coexist only checks that the derived filenames differ. Please add a test that builds the dataset with sampling_rate=500 and checks two things. First, that signal_file points at records500/..._hr. Second, that load_ptbxl_record returns shape (12, 250). A set_task test at 500 Hz would also work.

Optional suggestions:

• superclass_names is defined on the task but never used. Either use it or remove it.
• The example runs split_by_strat_fold without folds=, so it decodes every waveform. It would be better to pass precomputed folds, as the helper's docstring recommends.
• The docstring says v1.0.1 and v1.0.2 are rejected, but only missing columns raise an error. Soften the wording or add a version check.
• With PYHEALTH_REQUIRE_CACHE_DIR set, files are written before BaseDataset raises. You describe this as intentional, so a short note in the docs may be enough.

Coverage: total coverage is 71.7%, above the 60% bar. Every changed file is above 80%: ptbxl.py 83.5%, tasks/ptbxl.py 93.4%, splitter.py 96.6%, datasets/__init__.py 83.3%, tasks/__init__.py 89.2%.

• Switched to status: manual: changes files outside docs/, examples/, pyhealth/, test-resources/, tests/ (pyproject.toml).

_{Reviewed commit 2271703}

[AxelNoun]

Thanks for the review. Both blocking points are addressed: a53669a fixes the sex encoding and 2cc8539 adds the 500 Hz tests. 5d874df covers the optional documentation points.

1. Sex encoding. The review is right. Wagner et al. (Scientific Data 7:154, 2020), Table 2, lists sex as "male 0, female 1". The loader always passed the values through unchanged, so only the documentation was inverted. It is now corrected in the PTBXLDataset docstring, in configs/ptbxl.yaml and in the comment in tasks/ptbxl.py. The fixture README does not mention the encoding.

2. 500 Hz path. I added two tests:

• test_500_hz_signal_file_points_at_records500 checks that every signal_file in the derived CSV lies under records500/ and ends in _hr. It does not need wfdb.
• test_set_task_at_500_hz_loads_records500 runs set_task at 500 Hz and checks that the signals have shape (12, 250).

Both tests fail if _write_derived_metadata is forced to use filename_lr.

Optional points

• Example: I added a comment saying that split_by_strat_fold without folds= decodes every stored sample, signal included, once. I did not switch the example to folds= read from the derived CSV. set_task drops records with empty label sets by default, so sample indices do not line up with CSV rows.
• Version wording: the docstring now states only what is verified. v1.0.1 and v1.0.2 still contain duplicate records that later releases removed (21,837 records in v1.0.1, 21,799 in v1.0.3). The docstring also says that the release version is not detected.
• PYHEALTH_REQUIRE_CACHE_DIR: the docstring now says that the derived CSV and the resolved YAML are written before BaseDataset raises. Where they should go in that case is still an open question (see my comment of 7 October).
• superclass_names: I kept it for now as the public list of the five official classes on the task. It is the same object as PTBXL_DIAGNOSTIC_SUPERCLASSES, so there is still a single source. I can remove it if you prefer.
• Label vocabulary: it stays data-dependent, as documented. Pinning it to the five classes would need a processor configured with those labels, which is a design choice I would rather leave to maintainers.

Locally, test_ptbxl.py (30 tests), test_dataset_cache_key.py (4) and test_default_cache_dir.py (5) pass.

[Logiquo]

/review

[github-actions]

AI review: findings

This PR adds PTBXLDataset (a BaseDataset subclass with a YAML config), PTBXLSuperclassClassification (5-superclass multi-label task), and split_by_strat_fold to splitter.py, plus docs, an example and an optional wfdb extra. It changes no base class or existing function, and PyHealth has no existing PTB-XL loader or superclass task; CardiologyDataset reads a different Challenge 2020 copy for binary tasks and is a deprecated stub, so the PR is not redundant. Tests cover helpers, metadata cache isolation, task label output, the splitter and the fold/patient-leak checks, with 28 passing cases on a synthetic fixture, and the set_task end-to-end tests are skipped unless wfdb is installed. No blocking problems were found. Non-blocking: the docs say releases before v1.0.3 are unsupported, but the loader does not detect them and would silently ingest duplicate records (ptbxl.py:309); metadata is written to the cache before the PYHEALTH_REQUIRE_CACHE_DIR check, a disclosed side effect that follows the EEGBCI pattern (ptbxl.py:400); the quickstart calls split_by_strat_fold without folds=, which decodes every 12-lead signal (example line 59); and drop_empty_labels=False is only tested at task-call level, not through set_task (test_ptbxl.py:345). Overall risk is low.

_{Reviewed commit 5d874df}

[github-actions]

[low] Unsupported PTB-XL releases are documented but silently accepted

The class docstring says v1.0.1/v1.0.2 are not supported because they contain duplicate records, but the loader only checks for missing columns, so a v1.0.1 root loads without warning and yields about 21,837 records including duplicates. Consider a record-count or version check that warns, or soften the wording to say the release is not validated. Non-blocking because the limitation is stated.

[github-actions]

[low] Derived metadata is written before BaseDataset's cache check

prepare_metadata() and _write_resolved_config() write to the default metadata cache before super().init(). With PYHEALTH_REQUIRE_CACHE_DIR set and no cache_dir, files are written to ~/.cache and then BaseDataset raises. The PR discloses this, and EEGBCI, TUAB and TUEV follow the same before-super pattern (eegbci.py:78), so it is consistent with existing code. Fix later by checking the env var before writing. Non-blocking.

[github-actions]

[low] Quickstart decodes every sample to read fold numbers

split_by_strat_fold(samples) without folds= calls dataset[i] for every sample, which decodes all 12-lead signals just to read strat_fold. The inline comment explains this, but a quickstart could show the cheaper folds= path or note the cost. Non-blocking.

[github-actions]

[low] drop_empty_labels=False is not tested through set_task

test_task_keeps_empty_when_disabled calls the task directly. No test runs set_task with drop_empty_labels=False, so the MultiLabelProcessor path with empty label lists is unexercised. The end-to-end set_task tests are also skipped without wfdb. Add one set_task case with drop_empty_labels=False that checks the zero vector for empty labels. Non-blocking.

[github-actions]

AI Review

suggestion: approve
decision: not available

Thank you for contributing PTB-XL support. The new dataset, the 5-superclass multi-label task and the split_by_strat_fold helper fill a real gap, since CardiologyDataset can't do this on the current API. The docs, the example and the synthetic-fixture tests make it easy to review. It looks ready to merge.

Coverage against the bar (80% per file, 60% overall):

• Whole repository: 71.7%.
• pyhealth/datasets/ptbxl.py (added): 83.5%.
• pyhealth/tasks/ptbxl.py (added): 93.4%.
• pyhealth/datasets/splitter.py (modified): 96.6%.
• pyhealth/datasets/__init__.py (modified): 83.3%.
• pyhealth/tasks/__init__.py (modified): 89.2%.

All of these meet the bar.

Optional suggestions:

• The docs say releases before v1.0.3 are unsupported, but the loader doesn't detect them. A v1.0.1 download would be read silently and would include the duplicate records. A light check, such as a row count or a duplicate-record check, could give a clear warning.
• The quickstart calls split_by_strat_fold without folds=. That decodes every 12-lead signal just to read the fold number. Collecting folds from the metadata and passing folds= would make the example much faster.
• drop_empty_labels=False is tested only by calling the task directly. A short set_task end-to-end test would also cover it.
• The metadata CSV and YAML are written before BaseDataset raises under PYHEALTH_REQUIRE_CACHE_DIR. You've documented this, so it isn't blocking, but it may surprise users.

_{Reviewed commit 5d874df}

[Logiquo]

LGTM, thanks.

[AxelNoun]

Correction to my previous comment about the example. Row order indeed cannot be used, but samples.record_to_index maps each record_id to its sample indices without decoding anything. In 244e561 the quickstart reads strat_fold from ptbxl_database.csv, aligns it through record_to_index and passes folds= to split_by_strat_fold. On the synthetic fixture, the split decodes no sample and gives the same folds as reading them through samples[i].