Icca data source

[Dr-awe]

A new new beginning for ICCA vizualisation

Add support for ICCA datasource

[AlexisJanin]

Ported review from #52.

These are the review comments I left on #52 on 2026-06-04. That PR was opened against prepare-public-release rather than main and was closed unmerged, so the comments never reached the branch that is actually going to land. The ICCA module files are byte-identical between #52's head (1ecf569) and this branch's head (e9666b0), so every anchor maps 1:1 and all seven points below still apply to the code as it stands.

Two notes on the port:

• One comment from #52 asked for example/template_patient_data_structure/icca/.gitkeep. That has since been added, so I have not re-posted it.
• The delimiter comment originally pointed at syringe/find_load_format.py; syringe was removed on dev (60a4adb, issue #74), so I re-pointed it at other/, which still uses the same csv.Sniffer pattern.

A separate review covering what has changed on dev since June follows.

[AlexisJanin]

None-sense to have .parquet here: icca datasource can be exported in multiple format, from which we currently only support csv, and parquet is not even a possible export option from icca

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch.}

[AlexisJanin]

Timezone localization conceptually belongs in _format, not _load — and the base _format already does it for us (it localizes whenever DATA_SOURCE_DEFAULT_TIMEZONE is defined, which it is here).

Why it matters: _load's result is written to the parquet cache, and quick_load reads that cache without calling _load. So localizing here freezes UTC into the cache, and a database_options timezone override is then silently ignored on cached runs (apply_timezone_to_dataframe skips an already-tz-aware index). Every other source caches a tz-naive frame and lets _format localize each run — the cache should have the same tz-awareness as the raw data, i.e. none.

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch.}

[AlexisJanin]

Same as comment about timezone in find_load_format: return a tz-naive empty index here for consistency — _format will localize it:

return pd.DataFrame(index=pd.DatetimeIndex([]))

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch.}

[AlexisJanin]

To "prepare the ground" for known future formats that may be normal for icca(e.g. xml, json, ...), keep an explicit branch that fails loudly rather than silently with an NotImplementedError. Keep this list explicit in the options file.

For format that won't ever be supported (e.g. .parquet) simply go with a format or value error (something relevant)

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch.}

[AlexisJanin]

Hardcoded constant must live in the options.py file.

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch. Originally anchored on line 18; re-anchored to the constants block itself (15-17).}

[AlexisJanin]

delimiter="," is hardcoded; other auto-detects with csv.Sniffer. Worth matching for consistency and robustness against ;-delimited exports — see other/find_load_format.py:49-58 for the pattern.

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch. The original pointed at syringe/find_load_format.py:26-31; the syringe source was removed on dev in 60a4adb (issue #74), so the reference now points at other/, which still carries the same sniffing pattern.}

[AlexisJanin]

Since the time column is literally utcmeasurementTime, a non-UTC timezone override is almost certainly a user mistake (it would localize true-UTC stamps as if they were local time). Consider warning when an override ≠ "UTC" is supplied, e.g. a small _format override that logs a warning before calling super()._format(...).

_{Ported from #52 (review of 2026-06-04), which was closed against the wrong base branch.}

[AlexisJanin]

Second review: what has changed on dev since June.

The ICCA module is byte-identical to what was written in June against the old base (879b305). The rebase onto dev moved the three shared files — registry.py, tests/datasource/conftest.py, tutorial.md — but never touched the module itself, so it has drifted from conventions dev established in the meantime. Everything below is verified against the code as it stands, not inferred.

Two data-integrity findings worth attention before merge, both invisible in the current tests because the demo file happens not to trigger them:

• aggfunc="first" drops 25% of the demo file's measurements, every collision holding two different values.
• Attributes with non-numeric values are deleted at load time, so inspect() cannot report them.

Three consistency items (deduplicate_then_sort_index, narrating comments) and four documentation gaps (README table + blurb, tutorial blurb + per-source note, CHANGELOG) follow.

Two notes on scope:

• The registry entry still carried the retired Callable[[dict, dict | None], list[Signal]] annotation instead of the MainModule alias — that one slipped through the rebase on my side, so I've fixed it directly rather than leaving a comment.
• Some of these patterns were inherited by copying mindray_respi_numerics, which has the same issues. Where that's the case I've said so — they are not mistakes made here, and a couple deserve their own issue against the siblings rather than being fixed only in ICCA.

Things I checked that are correct and need no change: demo time alignment (ICCA lands at 10:12:38 CEST, inside the common window from 49d74ca, despite never being part of that pass); column pruning, datetime pushdown and display_fallbacks, all of which work via the base class with nothing for ICCA to declare; and the omission of ALLOW_QUICK_LOAD / CREATE_SOURCE_SYMLINK / ALLOW_DATETIME_PUSHDOWN, whose defaults are right for this source.

[AlexisJanin]

aggfunc="first" is silently discarding a quarter of the demo file's measurements.

Duplicate (utcmeasurementTime, attributeId) pairs are collapsed to a single value and nothing reports it. Measured on the committed PtHighDensityAnesthesiaData_SYNTH.csv:

rows=500  unique(time,attr)=375  collapsed=125 (25%)
collision groups with >1 DISTINCT value: 125 of 125
attributeId 2423: 140 raw rows -> 79 points after the pivot

The important part is the second line: every one of the 125 collisions holds two genuinely different valueNumber values (e.g. 2423 at 08:12:54.003 carries both 80.90 and 83.28). This is not de-duplication of repeated readings — it is dropping real measurements. utcChartTime collides identically, so switching the time column doesn't help.

The docstring at lines 34-35 acknowledges the collapse, but a user has no way to learn it happened, and downstream inspect() reports the post-pivot count as if it were the file's content — so a 25%-loss export looks clean.

Two suggestions, the first cheap and worth doing regardless:

1. Count and warn:

   collisions = df.duplicated(subset=[COLUMN_TIME, COLUMN_ATTRIBUTE_ID]).sum()
   if collisions:
       logger.warning("[%s] %d duplicate (time, attributeId) pairs collapsed by aggfunc='first'", cls.DATASOURCE_NAME, collisions)

2. Decide deliberately whether first is the right rule. A real export carries valueInstanceId, deviceSourceId, isValid and isDisplayValue (all flattened to SYNTHETIC/1 in the demo file) that would normally disambiguate these. Worth checking with whoever produced the export before settling.

[AlexisJanin]

Attributes whose values aren't numeric disappear before inspect() can report them.

The export has 44 columns, including valueString, valueDateTime and utcValueDateTime. Reading only valueNumber means an attributeId whose values are strings — ventilator mode, drug name, a free-text remark — pivots to an all-NaN column, and pivot_table's default dropna=True then removes that column entirely:

# same frame, only the flag differs
dropna=True  (current) -> pivot columns: [111, 222]
dropna=False           -> pivot columns: [111, 222, 999]   # 999 is the string-valued attribute

Why this one bites harder than it looks: inspect() is what users reach for to discover what's in a file, and it deliberately strips field_display upstream (base.py:756-761) precisely so it can report every column. For ICCA it can't — the attribute was destroyed inside _load, before the cache was even written. Someone configuring field_display gets "no such signal" and no way to learn the attribute exists.

The shipped demo file has zero valueString and zero valueDateTime rows, so the example data and both golden snapshots conceal this completely. It will first appear on a real export.

Suggested minimum: pass dropna=False so the attribute stays visible as an all-NaN column, and log once at load time listing the attributeIds that had a non-null valueString/valueDateTime but no valueNumber. Whether ICCA should eventually support string-valued signals is a larger design question — but the silence is the defect.

(Note: dropna=True also drops index rows that are all-NaN. Those carry no numeric data by definition, so nothing is lost that way — that half is fine.)

[AlexisJanin]

Use the shared helper: deduplicate_then_sort_index from clinical_scope.io.file_utils.

1883a2c ("unify dedup-then-sort process everywhere both are used") introduced it and replaced exactly this two-liner in mindray_respi_numerics/find_load_format.py — the module ICCA was copied from — as well as in servo_u. edf and mindray_scope use it too. ICCA is the last hand-roll left.

It also runs the two steps in the opposite order to the helper, which its docstring calls out deliberately: deduplicating first keeps the first row in file order on a timestamp collision, whereas a non-stable sort_index() beforehand decides that arbitrarily.

Being straight about the scope: pivot_table already returns a unique, sorted index, so line 74 is a no-op today and no test can currently distinguish the two orders. It becomes live only if pd.to_datetime at line 68 maps two distinct index strings to the same instant — mixed formatting in the export, e.g. ...38.993000 alongside ...38.993. Note the pivot at line 57 groups by the raw string, so that's the one path where the export's text formatting leaks into grouping. Then the current order silently keeps one row and drops every attribute value that existed only in the other.

from clinical_scope.io.file_utils import deduplicate_then_sort_index
...
df_pivoted = deduplicate_then_sort_index(df_pivoted)

[AlexisJanin]

Six comments in this method narrate what the next line does rather than explain why: # Load the data (40), # Pivot the data: … (55), # Convert index to datetime (67), # Sort by timestamp (70), # Remove duplicate timestamps (keep first) (73), # Apply timezone if needed (76).

This isn't a style preference invented for this PR — 4cfbd7e deleted the near-identical set from mindray_respi_numerics/find_load_format.py, the module this one was copied from:

-        # Load the data
-        # Pivot the data: one column per measurement type
-        # Convert index to datetime
-        # Sort by timestamp and remove duplicate timestamps (keep first)
-        # Apply timezone if needed

That sibling is now down to a single comment. CLAUDE.md states the rule: "explain the non-obvious why, not the what." Line 55 also just restates docstring line 32.

Please keep lines 63-64 — "attributeId is an integer; use string column names so they round-trip through parquet and resolve as raw signal names in database_options" is exactly the kind of why that should survive; nobody would reconstruct that from the code.

[AlexisJanin]

[Unreleased] still reads _Nothing yet._ — a new datasource is user-visible surface and needs an entry. The precedent is EDF's bullet under 1.1.0 Added (line 28):

• .edf datasource. Reads European Data Format recordings, the usual container for EEG and polysomnography exports.

Suggested:

### Added
- **ICCA datasource.** Reads Philips IntelliSpace Critical Care and Anesthesia high-density
  anesthesia exports, pivoting the long-format rows into one signal column per `attributeId`.

[AlexisJanin]

The Supported Data Sources table has no ICCA row. All nine other registered sources are listed, and EDF got its row when it was added — this table is one of the places that rots silently when a source is added.

Suggested insertion after this line (before Other (Generic), matching DataSource.AVAILABLE order):

| ICCA | Philips IntelliSpace CCA anesthesia export | `.csv`, `.parquet` | Anesthesia high-density signals (HR, SpO₂, ABP/NIBP, airway pressures) |

Note the extension column here should stay in sync with options.py's FILE_EXTENSIONS and with the tutorial's table row — see the separate thread about whether .parquet belongs in that list at all.

[AlexisJanin]

This intro blurb lists the devices by name — "Servo-U ventilators, EIT systems, FluxMed, Mindray, EDF recorders" — and ICCA is missing. The canonical table further down is the thing that must be right, but this sentence is what a new reader actually sees first.

Same omission in docs/user_guide/tutorial.md:25 ("Display signals from FluxMed, Mindray, EIT, Servo-U and EDF recorders simultaneously") — flagged separately there.

[AlexisJanin]

"Display signals from FluxMed, Mindray, EIT, Servo-U and EDF recorders simultaneously" — ICCA needs adding to this list. (Same omission in README.md:28.)

[AlexisJanin]

ICCA needs a per-source note here, after the EDF one. Of everything in this review this is the one I'd argue hardest for, because without it the source is close to unusable for a clinician.

ICCA's raw signal names are stringified integer attributeIds — 2423, 19602, 4347. There is no name dictionary anywhere in the export: terseForm and verboseForm only repeat the value and its unit. So someone opening an ICCA file has no way to guess what to put in field_display or signals, and no way to find out from the file itself. Every other source in this guide has human-readable raw names, so nothing prepares a reader for this.

Two facts to cover, mirroring the shape of the EDF note above:

• Signal names are attributeId integers, as strings. Point readers at inspect (or the demo config at example/demo_database/database_options.json) to discover which ids a given export contains, and note that mapping an id to a clinical meaning needs the site's own ICCA dictionary.
• Timestamps come from utcmeasurementTime, i.e. UTC, which is why this source defaults to UTC rather than Europe/Paris like the others — overridable via additional_informations.timezone.

Worth saying explicitly that the ids are opaque by design of the export format, so nobody files it as a bug.