Release v1.1.0: spectrogram/PSD, EDF, user options, per-file other:: config - #64
Open
AlexisJanin wants to merge 57 commits into
Open
Release v1.1.0: spectrogram/PSD, EDF, user options, per-file other:: config#64AlexisJanin wants to merge 57 commits into
AlexisJanin wants to merge 57 commits into
Conversation
…ere both are used
Improve loading time and memory
display_timezone's only load-path job was interpreting a naive datetime_start/ datetime_end; storing the bounds as tz-aware instants instead removes that job entirely, so display_timezone becomes a pure render concern (unblocks #69). - timezone.py: to_aware_display_ts (inverse of to_naive_display_ts) plus a shared resolve_display_timezone coercion so a hand-edited or programmatic bad timezone name falls back and logs instead of raising inside pandas. - Submit bakes the form's naive text into an aware instant before saving; Reload renders it back as naive wall-clock in the saved file's own display_timezone; a new callback rewrites the datetime fields when display_timezone itself is edited, so the instant survives the change. - eit/find_load_format.py: strip the offset before treating datetime_start as the inferred recording day, preserving today's calendar-day behaviour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ht and focused on display
… to each of the source file (issue 75)
…tasource (issue 74) - find_files keeps one file per stem, preferring parquet, so a folder holding both data.csv and data.parquet no longer loads the same signals twice - 'other' now symlinks each source file into clinical_scope_output/, extending the traceability guarantee previously limited to philips_waves - an other::<stem> section can carry a trace_options block (mode, line_width, ...) overriding the datasource default - xlsx global groups emit qualified refs for other:: signals, which could not resolve before
… 74) They performed no format-specific parsing, so they were 'other' with extra machinery. Now that each file in other/ carries its own configuration and time_shift, they no longer earn a module. Example data moved into other/, configs rekeyed to other::<stem>, and a folder still named after a removed source is reported with a warning. Also corrects signal names in the demo workbook that never matched the demo data (a pre-existing mismatch this migration made visible).
Extract shared type-check and window/overlap-override boilerplate from Signal.spectrogram_from_signal/psd_from_signal; move hovertemplate dB/Hz precision into constants.Spectral; drop an issue-number reference from a comment; close test gaps around the sentinel-only-column warning loop (only trace_mode was covered; timezone had no coverage at all), trace_options isolation across datasources, and characterize today's unvalidated pass-through of trace_mode/line_width/opacity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
example/ mixed shipped demo material with test-only fixtures. It now holds
only what a user or the release bundle touches; test fixtures moved under
tests/data/.
Moves (history preserved):
- Patient_difficult_format -> tests/data/patients/
- example_database_options.{json,xlsx} -> tests/data/option_files/
Removals:
- Patient_other: unreferenced; two files were byte-identical to demo_patient,
and its stem-collision case is already covered synthetically in test_other.py
- example_database_options_other.{json,xlsx}: superseded by the demo config
- template_database_options.json: restated the tutorial reference with no
consumer and had silently drifted three features behind
example/demo_database/database_options.json is new: a generated twin of the
xlsx, so library users who never launch the app have a runnable config to
point at. The xlsx gained sections for fluxmed_signals, fluxmed_parameters,
mindray_respi_waves and mindray_respi_numerics -- the demo shipped data for
these but never plotted them -- plus range/visible/display coverage. Every
column of the signals sheet is now exercised by a real example.
New guards in tests/unit/test_example_assets.py: the folder template matches
the registry (this is how edf/ went missing), each template folder keeps its
.gitkeep, the demo json matches its xlsx, and the demo config configures every
datasource it ships.
Docs: tutorial repointed at the demo configs and its display/visible rows now
state how they differ; CLAUDE.md corrects the database_options key name to
'signals'; README names a real config path; the new-datasource skill requires
a demo config section; the organize-patient-folder evals no longer target a
patient folder deleted a release ago.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The demo's anonymisation left two stamping conventions in the raw files (naive 08:12:33 and 06:12:33 UTC), and each source's DATA_SOURCE_DEFAULT_TIMEZONE read them differently, so one recording landed in three clusters two hours apart. Loading the demo showed sources that never overlapped. Data shifted +2h so it agrees with the sources that were already right: - mindray_scope: all seven CSVs re-stamped; filenames renamed to keep the range they advertise truthful (they only supply the signal name to the loader, which reads timestamps from the file contents) - servo_u: the 'Log start' header of Curves_00000000.sta, which is the sole reference -- timestamps are built as Log start + Time(ms), and the T(h:m:s.ms) column is dropped - other/numerics.parquet: tz-aware index 06:12:33Z -> 08:12:33Z, since an already-aware index ignores the database_options timezone override Config instead of data where the stored index is naive: other::waves moves from Europe/Paris to UTC. The demo now exercises four timezone paths at once -- explicit UTC, explicit non-European (eit stays Asia/Karachi, landing 21 minutes after the others), implicit source default, and already-aware-so-override-ignored. Ten of eleven sources now start at 10:12:33 CEST; eit remains offset so the demo still has something for time_shift to correct. patient_options_example.json and the README quickstart now carry the demo's real eit day and edf recording_start, so 'try it immediately' holds. Snapshots regenerated for mindray_scope and servo_u. The naive/aware bound equivalence test pinned two windows seven seconds into the servo_u recording; both move +2h with the data, keeping the same instant expressed two ways. Full suite green: 809 passed, ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spectrogram & PSD plot types, EEG datasource, other-source cleanup
CI installs ruff unpinned and had moved to 0.16.2 while local venvs sat on 0.15.x, so the format step failed on four files and had masked the lint and test steps entirely — they had never run on this branch. - Reformat the three Python files that no ruff version accepted, one of which also broke E501, plus build_info/README.md: ruff 0.16 formats Python code blocks inside Markdown, which no 0.15 release did. - Ignore CPY001. `select = ["ALL"]` swept in the new missing-copyright rule and it fired on all 66 modules; attribution lives in LICENSE, not per-file. - Parenthesise the implicit string concatenations in the native-license map. Adjacent strings sat beside real tuple elements in a dict[str, tuple[str, str]], where a missing comma would silently widen the tuple and corrupt attribution. - Drop ten `# noqa: BLE001` directives that 0.16 made dead: it no longer flags a broad except whose handler logs with exc_info. Verified with ruff 0.16.2: format --check and check both clean; full suite passes on Python 3.11.15 and 3.13.13 (841 tests each). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The [Unreleased] section only covered the datasource removal (#74/#75); the other ~40 commits since v1.0.0 were undocumented. Write them up and cut the release entry. Bump to 1.1.0 in both pyproject.toml and CITATION.cff — the release checklist tracks both, and CITATION.cff had drifted, still reading 1.0.0 against a pyproject already at 1.0.1. 1.1.0 rather than 2.0.0 for the datasource removal: the break is real but narrow, and the migration is a rename inside two config files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are interactions between the new user_options tier and code that predates it, and both fail silently. Hovermode: render_annotations patched layout.hovermode to a hardcoded literal while PlotModel.to_figure sets it from DisplayFallbacks. The callback fires on any annotation create/edit/delete or mode toggle, so choosing "Closest point only" in Settings held until the user touched an annotation, then reverted for the rest of the session. The comment claimed the two stayed in step; the capability set was shared, the value was not. Read it from the store instead — point mode still forces "closest", which is what placing a point needs. Spectrogram dB range: each bound was clamped independently with no ordering check, so a min above max survived to go.Heatmap as zmin > zmax. Reachable by typing two valid numbers into Settings (100 then 40), and also by the clamp itself, which turns an out-of-range pair into (100.0, -100.0). Add ordered_bounds, which falls back to the schema defaults and says so in the log. Also close the FLOAT hole in _coerce: it clamped INT only, while its docstring promised "numeric fields are clamped". The two FLOAT options are exactly the spectrogram bounds above. Adds 7 tests, including the positive cases so the fallbacks cannot pass vacuously. Full suite 848 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An ADR audit over the 49 commits found the reasoning was not skipped, it went into issue bodies instead — #57 literally opens "This issue is the decision record + spec". Those issues are closed and #57 has already drifted, since it specifies pushdown for philips_waves, which this same branch deletes. Three decisions cleared the bar (a real alternative was available and rejected, and the decision constrains future work): - 0007, read-time pruning is an optimization, never a filter. The rejected alternative was exact pushdown, which loses rows silently because time_shift and timezone resolution run after load. Also records why inspect does not push down: its "% retained" is a comparison against the unwindowed file, so pushing the window into the read makes the number a tautology. - 0008, a datasource module is justified only by format-specific parsing. The criterion was in CLAUDE.md as bare fiat; CONTRIBUTING.md now carries it too, which is where a contributor actually looks before writing one. - 0009, other::<stem> is a config scope and :: is the qualified-name separator. Records why the per-file scope is deliberately NOT generalised to the other sources, whose folders hold chunks of one recording rather than unrelated files. Decisions deliberately left without an ADR: the capability-set PlotType design, the edf addition, the example/ split, demo time alignment, source symlinks and stem dedup — mechanical, precedent-following, or already covered. Also fix the PSD example in the tutorial: db_range [40, 90] predates the spectral density re-scale and now sits entirely above the data, which measures 6.5-41 dB on the demo EEG. [0, 40] matches the two spectrogram examples the re-scale did update. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AlexisJanin
changed the title
Collaborator
Author
Three-axis review —
|
| Axis | Findings | Worst in that axis |
|---|---|---|
| Standards | 9 | Hand-spelled Dash callback IDs that break silently on rename |
| Spec | 9 (7 accepted) | PSD db_range documented above the data — now fixed |
| Maintainability | 8 | Hovermode silently discarding a user setting — now fixed |
The three I'd fix first, from anywhere: the trace_options accept/interpret gap (ten scopes accept, one reads); the hand-spelled user_options.display_timezone callback IDs; and the tests that restate their own constants, which are the ones least likely to be caught by anything else.
Merge readiness: CI green on both matrix Pythons, 848 tests passing, no ADR violated, breaking change documented with a migration path. Nothing outstanding blocks the merge.
Collaborator
Author
|
Follow-ups from the review above are now tracked:
The 📋 markers in the review map onto these three. |
Pass over the 48 Python files this release touched. 935 comments examined, 13 removed, 16 rewritten, 906 kept — the codebase's comment density is mostly earned, so the bar was "delete only what restates the line below it". Removed the repo issue tags per house rule, rewriting each to carry its reason in prose instead of behind a link: base.py (x2), constants.py, eit/options.py, registry.py, bench_pushdown.py (x3). "see issue #53" became "one diagnostic per patient would flood the log". Two worth calling out: - constants.py had a commented-out fake `class DataSourceRelative` standing in for documentation. Replaced with a real one-line pointer to where per-datasource patient options actually live. The knowledge survived. - eit/options.py asked for names to move to example_eit_options.json, a file that exists nowhere in the repo — an aspiration pointing at something that never shipped. Deliberately kept: the dash PR #3785 reference (upstream, and the only way to know when the zoom workaround can go), the tz-naive/tz-aware folklore in mindray_scope, and ~90 section banners, which are a house convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Most markdown here grew substantially during 1.1.0 and had started repeating
itself across files. 106 lines removed, no facts dropped.
Cross-file duplication resolved by deferring to the canonical source:
- README restated the tutorial's "canonical" datasource table one line above a
link to it. Kept device/format/extensions as front-page orientation, dropped
the folder-keyword column to the tutorial.
- CONTRIBUTING was linked from README three times over — prose line, a
Documentation table, and its own section. The table went.
- The module-justification rule sat in CONTRIBUTING, CLAUDE.md and ADR-0008.
The summaries keep the test and a link; the rationale lives only in the ADR.
- output_root's one-root-per-database warning appeared three times in the
tutorial; the explanation stays in one place, the rest cross-reference.
One substantive correction, verified against the code: the `numerics` block was
documented as applying to "all numeric parameter signals". It is read in
_build_trace_options (signal_container.py:421), which runs for every signal with
no filtering — the name misleads and the docs repeated the mistake. Fixed in the
prose and in both table rows that echoed it.
Also fixed three typos that were factual noise ("providem uch", "runned") and
dropped a machine-specific `cd ~/Codes/ClinicalScope` from tests/README.md.
Left long on purpose: the `::` shadowing paragraph, the spectral-density
rescale, and CHANGELOG.md entirely — its migration notes are the only path
users get. The ADRs needed no cuts; their length is the rejected-alternatives
reasoning the format exists to carry.
Co-Authored-By: Claude Opus 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.
Merges the accumulated
devwork intomainas v1.1.0 — 51 commits since v1.0.0, touching 163 files.What's in it
Two new plot types. Spectrogram and PSD, for high-rate signals (EEG, pressure waveforms). Both are re-renderings of the same samples, which is the side of the ADR-0006 line this library stays on.
A
.edfdatasource — European Data Format, the usual EEG/polysomnography container.A third configuration tier,
user_options. Per-person app behaviour and display fallbacks, edited in the Settings modal. It only ever fills gapsdatabase_optionsleft — it never overrides (ADR-0005).Per-file configuration for
other/. Each file gets its ownother::<stem>scope with its owntime_shift, timezone, grouping and trace style (ADR-0009).Breaking:
philips_waves,philips_numericsandsyringeare removed. They did no format-specific parsing, so onceother/gained per-file scopes they no longer earned a module (ADR-0008). The changelog carries the migration, including three traps: the syringe timezone default moves Paris → UTC, marker traces need restoring viatrace_options, and signals take the qualifiedother::<stem>::<column>form.Substantially faster loading. Columns are selected and pruned before reading, with row pushdown as a strictly optional accelerator (ADR-0007).
Full detail in CHANGELOG.md.
Pre-merge hardening on this PR
CI had been red since 2026-07-29 — 13 consecutive failures. Two independent causes, both in
ruff format --check:E501).dev = ["ruff"]is unpinned, so CI had moved to ruff 0.16.2 while local venvs sat on 0.15.x. 0.16 formats Python code blocks inside Markdown (a 4th file) and, underselect = ["ALL"], addedCPY001— 66 hits across every module.Because the format step fails first and
fail-fast: truecancels the sibling job, lint and tests had never once run on this branch. They do now: 848 tests pass on Python 3.11.15 and 3.13.13.Also landed here:
user_optionsand code predating it — see the review comment below.db_rangein the tutorial corrected against measured demo data.pyproject.tomlandCITATION.cff; the two had drifted (1.0.1 vs 1.0.0) anddocs/RELEASING.mdtracks both.Review
A three-axis review (Standards / Spec / Maintainability) was run over the full
main...devdiff; findings and their triage are in the review comment on this PR.