Skip to content

[CPBench] Benchmarking Wrapper#1168

Open
lehendo wants to merge 5 commits into
sunlabuiuc:masterfrom
lehendo:cpbench
Open

[CPBench] Benchmarking Wrapper#1168
lehendo wants to merge 5 commits into
sunlabuiuc:masterfrom
lehendo:cpbench

Conversation

@lehendo

@lehendo lehendo commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

tbd. will thoroughly update because I completely redid this.

@fbonc

fbonc commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Hello ac!

Main things I ran into, as requested:

1. The wrapper does not fully match current PyHealth APIs

A few crashes came from the wrapper assuming all datasets/tasks/models are built the same way.

E.g., the wrapper tries to do something like dataset.set_task(task_fn) where task_fn is a separate function that turns raw records into ML samples. That does not seem to work uniformly (anymore?).

SleepEDF needed the default task path instead: dataset.set_task()

ISRUC failed because its dataset object did not have set_task at all:
AttributeError: 'ISRUCDataset' object has no attribute 'set_task'
ISRUC is also listed as supported, but crashes due to this.

SparcNet also failed because the wrapper passed arguments that current SparcNet does not accept:
TypeError: SparcNet.__init__() got an unexpected keyword argument 'feature_keys'

So I think the main issue is that the wrapper is written against a mixed PyHealth interface which not all datasets and models use. Note: didn't test against all interfaces so there's likely more - not a comprehensive list above.

3. SleepEDF preprocessing hit a data edge case

SleepEDF crashed because one recording did not contain “Sleep stage 4”:
ValueError: No matching events found for Sleep stage 4

Adding on_missing="ignore" to the MNE Epochs(...) call let it continue.

4. The script prints results but can fail when saving them

After local patches, I got a capped SleepEDF run to print a result table, but then saving failed because the output folder did not exist:

FileNotFoundError: 'results/sleepedf_label_smoke.json'

Probably just needs to create the parent directory before writing the JSON.

5. Maybe a test mode would be useful?

SleepEDF processed around 415k samples, so every wrapper bug took a while to reach. I added a local cap like 1000 samples per split, which made it much easier to debug the full path quickly.

7. The PR mentions checkpoint loading, but I did not see it in the CLI

Would be helpful because CP debugging should ideally be possible without retraining the
model every time.


Happy to help with the wrapper if you'd like. Peace.

@fbonc

fbonc commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Hello.

A few things:

  • Not really directly to do with the wrapper, but will likely still affect benchmarks: score_type="aps" still computes 1 - p(true class) on base_conformal, so it is LABEL rather than APS. I think Daryl caught this one in the past?
  • LABEL raises NotImplementedError when model.mode != "multiclass", so both MIMIC IV mortality tasks can't be ran.
  • save_results() opens the output path without creating its parent dir first, which crashesif the dir doesnt exist and therefore discard a run completely right at the end.
  • --dev silently overrides explicitly passed --alpha and --seeds
  • save_results() never writes split_strategy into the record, so by_patient and by_sample runs are indistinguishable in the output
  • Nothing checks that the declared tables cover the event types a task calls get_events for, so a missing table crashes after the full ETL instead of failing instantly. A pre-check would catch it, and --dry-run currently does not.

bye!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants