Architecture deepening: six structural refactors (ADR-0011 → 0014) - #90
Merged
Conversation
`_load` now takes only the file path and returns a frame; DataSourceBase writes the cache from it, at one site instead of in eight sources by convention. This is the structural form of ADR-0010 — a source cannot resolve an option inside `_load` because no option is in scope there. Nine `**kwargs` + `noqa: ARG003` pairs go with it. The base declines to cache an empty frame, and does not create the output folder for one either. Four sources' early returns already skipped the save for that reason; the rule is now uniform and stated once. Removes the `configured_field_display` guard ADR-0010 flagged for removal: its fresh-load branch could never fire under the rule. The parameter survives on the quick-load branch, where it still prunes the cache read serving inspect(configured_columns_only=True). test_load_config_independence changes kind rather than level. Config independence is no longer a behavioural property — both runs of a load now take an identical path, and a module constant does not vary between them, so comparing two runs would assert something that cannot fail. It reads each `_load` through the AST instead and checks the rule's two channels: the signature takes only the file path, and the body references neither DATA_SOURCE_DEFAULT_TIMEZONE nor apply_timezone_to_dataframe. Snapshots unchanged: every source saved exactly the frame it returned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cst.DISPLAY_TIMEZONE was serving two unrelated concepts: the default of the display_timezone user option, and the timezone a tz-naive datetime_start/datetime_end is interpreted in on the load path. Reading the second through the name of the first makes the load path look like it wrongly ignores the user's setting -- a misreading an architecture review made, and then made again while implementing the fix. It does not ignore it. The UI qualifies its bounds at Submit, so the Settings timezone governs the window there; both load-path sites localize only when tzinfo is None, and are unreachable from the app. The constant is a fallback for bounds that never met a user: scripts, library calls, hand-edited files. Resolving a user option for those would make extract_* output depend on ~/.clinical_scope/user_options.json. Split out cst.NAIVE_BOUND_TZ as a separate literal, not an alias -- aliasing would let a change to the app's display default silently reinterpret every script's naive bounds, which is the coupling the ADR forbids. filter_data_by_timestamps' parameter follows the concept, and resolve_display_timezone gained a fallback so the load path's invalid-name branch cannot land on the display default. Load-path tests monkeypatch the new name; the one site still patching DISPLAY_TIMEZONE covers inspect()'s cosmetic date ranges, which is genuinely the other concept. Behaviour is unchanged -- both constants are "Europe/Paris". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
file_utils.py held seven unrelated concerns behind one name and had the second-highest fan-in in the package. Its call graph is a stack, not a partition: parquet pruning calls into datetime detection, sharing four helpers. Split accordingly, one file per ADR: io/time_axis.py ADR-0004's rule plus both its adapters io/parquet_pruning.py ADR-0007: plan the prunings, then execute one io/discovery.py find_files and the folder predicates io/column_patterns.py one definition of what a `*` matches io/export.py save_df + print_out_figure Schema-only detection stays with full-frame detection: the two must pick the same column, and that invariant is only cheap to hold in one file. Provenance is now structural. `read_parquet_pruned(index_is_time_axis=)` became two front doors -- read_parquet_pruned for a file of unknown origin, read_cache_pruned for one we wrote -- so ADR-0010's guarantee is carried by the name a caller reaches for instead of by a comment saying "no other caller may claim that". _build_datetime_row_filters now converts a bound into the column's tz before dropping the label, rather than trusting _pushdown_bounds to have converted already. Same behaviour, one less contract held at a distance; the branch had no coverage, and now has a test that fails without it. Also removed: load_csv_with_datetime_index (no callers), load_parquet_with_datetime_index (both callers used the bare path, now inlined), and timezone.py's dead _first_last_timestamp twin. Hard cut, no compatibility shim. Test assertions are unchanged except where a deleted wrapper had to become the code it contained. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Detection samples candidate columns by name, but _sample_parquet_columns let pyarrow restore pandas metadata -- so a materialized index column came back as the frame's index and `sample[column_name]` raised KeyError. Any plain parquet with a stored non-temporal index (a float or int index, not one of our own caches) crashed instead of declining to prune. Predates the io/file_utils split; found by the tests added here. Reading with ignore_metadata=True keeps every requested column addressable by name. Our own caches are unaffected: they declare provenance through read_cache_pruned and never reach detection. Tests cover the pruning decisions through the two readers rather than through the plan they produce: the plan's shape is implementation, and "a row predicate was built" is an optimization under ADR-0007, not behaviour. Expectations come from a full read plus a plain pandas slice, never from replaying the library's own resolution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grouping and derived-plot construction were pure domain logic reachable only by running the whole pipeline against a folder on disk. They now live in plot_assembly.py, called once after the datasource loop, and are exercised with in-memory Signals and literal config dicts. Two rules replace the two-path shape (ADR-0013): Config scope is desugared once. A per-datasource section is a namespace, not a different kind of grouping, so its references are resolved against that datasource's own signals and re-emitted as qualified global ones before anything else runs. Downstream there is one resolver, one suppression rule and one spelling of a reference. Both config spellings stay valid: no parser change, no config migration. Group membership joins on signal identity. A raw name is unique only within a datasource, so the string-keyed accumulator and the post-hoc prune each dropped a plot the first time two sources shared a name -- ordinary for HR, SpO2 and ABP, and silent when it happened. Derived signals are new objects that were never in the input list, so they are structurally immune to suppression and need no naming disguise. Three deliberate behaviour changes, all in CHANGELOG: same-named signals in different datasources stop suppressing each other; a group that resolves to one signal keeps the group's name; and local loop / spectrogram / grouped_fields resolve display names. wrapper.py 931 -> 562 lines; main gains a docstring and loses the outer try/except that wrapped the moved steps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestMainGlobalGrouping guarded its only assertion behind
`global_fields & all_signal_names`, an intersection that is provably
always empty: an 'other' signal's raw_name is `<stem>::<column>`, so the
set holds `numerics::PNId`, never the bare `PNId` the guard compared
against. The assertion had never run. It is replaced by three that pin
what global grouping is for -- a group spanning three datasources, a
three-segment `other::<stem>::<column>` reference resolving, and a
grouped signal not also being plotted on its own.
The `pytest.skip("No time_series models produced")` guards go with it,
here and in its siblings. demo_patient/ is a fixed committed fixture, so
"maybe there is no data" is not a real condition -- it only meant the
tests would fall silent if the fixture ever regressed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The user_options schema rules -- MIN/MAX, CHOICES, valid IANA name -- were walked twice, in the settings modal on the way in and in DisplayFallbacks.from_user_options on the way out, and on neither occasion at load. A hand-edited ~/.clinical_scope/user_options.json therefore reached the store raw: two copies of one rule set, and the path that most needed them had none. They now live once, in a new core module user_options.py, called by every boundary that accepts a value. The module is pure -- no Path.home(), no file reads -- which is what makes wrapper.main's "the core never reads the on-disk file" structural rather than a matter of nobody calling the function. Disk I/O stays in helper_api. validate(raw) returns (clean, list[Correction]) rather than logging. Detection is shared; the reaction belongs to the boundary that knows the user's situation -- the modal discards silently because its widget re-renders showing the corrected value, the loader logs because nobody is watching a widget. That collapsed persist_user_options' corrected_a_widget flag to bool(corrections): it existed only to re-derive what per-field coercion had thrown away. from_user_options becomes a projection that converts but does not check. resolve_display_timezone stays, as the one tenant whose bad value raises inside pandas/zoneinfo instead of merely rendering oddly, covering a dict a library caller hand-built. Unknown keys warn and drop, three lines in the loader. Deliberately opposite to ADR-0012's Annotation.extra passthrough: the discriminator is provenance. Annotations are human-authored and get shared, so an unknown key is someone's data; user options are per-person state this app writes, so an unknown key is a value stranded under a name the schema no longer has. Two behaviour changes, both in CHANGELOG: a bad value in a hand-edited settings file is now corrected and reported at load, and the modal refuses an inverted spectrogram dB pair on save rather than leaving the render layer to fix it -- the cross-field rule moved into validate with the rest. validate also always returns every schema field, so the store can no longer be partial. Validation tests move to tests/unit/test_user_options.py and assert on returned Correction values, not caplog prose, which no longer breaks on a reworded message. src is +48/-144 across three files, +150 in the new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AlexisJanin
added
the
Code quality
… ref from docstring test_user_options.py compared corrected display_timezone against cst.DISPLAY_TIMEZONE, so the assertion could never fail independently of the constant it exercises. Also drops an issue-number reference from a test_time_axis.py docstring, keeping the rationale in prose. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eight commits from the architecture-deepening pass. Each one was designed before it was written, and each records its decision as an ADR rather than in this description — the ADRs are the durable artefact, this is a map to them.
66 files, +3477 / −2000. Seven new modules in
src/, four new ADRs, two updated.What is in it
56197fc_load(file_path) -> DataFrame— the base template owns the parquet cache write, guarded on a non-empty frame.otheropts out by never entering the template.90a232ecst.NAIVE_BOUND_TZsplit fromcst.DISPLAY_TIMEZONE, so changing the app's display default cannot reinterpret a script's datetime bounds.c50e1e1AnnotationSet+Group(a group carries its annotations, so an empty one is unrepresentable),Annotation.create,Annotation.extrakey passthrough. Group logic had zero coverage before.e15f00cio/file_utils.pysplit into five modules —time_axis,parquet_pruning,discovery,column_patterns,export. Three dead functions deleted; no compat shim.7acc156KeyErrorwhen detection sampled a stored non-temporal index — pandas metadata restored a materialized index. Predates the split; found while testing it.7079055plot_assembly.py: config scope desugared once into qualified global refs, and group membership joins on signal identity rather thanraw_name.86099f6TestMainGlobalGrouping's only assertion was behind a provably-empty guard and had never run. Replaced by three that assert.31d5783user_options.py: one pure implementation of the schema rules, called by every boundary that accepts a value. Load-time validation existed nowhere before.Behaviour changes
All user-visible ones are in
CHANGELOG.mdunder[Unreleased]. In short: same-named signals in different datasources stop suppressing each other; a group resolving to one signal keeps the group's name; per-datasourceloop/spectrogram/psdresolve display names;trace_optionsapplies to every datasource, not onlyother::<stem>; the annotation colour picker stops disagreeing with itself; and a hand-editeduser_options.jsonis checked when it loads.Reviewing
The ADRs carry the reasoning, including what was rejected and why. Commit messages carry the mechanics. Two things worth knowing while reading the diff:
e15f00cespecially) — a rewritten assertion stops a green suite from being evidence that a refactor preserved behaviour. Where assertions did change, the commit message says so and why.== 300, not== cst.DEFAULT_SUBPLOT_HEIGHT), per the project convention: a test that restates the constant it exercises can never fail.Opened primarily to get CI (ruff + pytest on 3.11 and 3.13) to report on the branch.
🤖 Generated with Claude Code