refactor: quality stats integrated into read models and tui#232
refactor: quality stats integrated into read models and tui#232
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| by_alias: bool = ..., | ||
| by_title: bool = ..., | ||
| exclude: set[str] | None = ..., | ||
| ) -> str: ... |
| by_alias: bool = ..., | ||
| by_title: bool = ..., | ||
| exclude: set[str] | None = ..., | ||
| ) -> list[dict[str, Any]]: ... |
| try: | ||
| with Session(engine) as session: | ||
| stats = get_event_quality(session, uuid.UUID(item_id)) | ||
| except (ValueError, SQLAlchemyError): |
| try: | ||
| with Session(engine) as session: | ||
| stats = get_station_quality(session, uuid.UUID(item_id)) | ||
| except (ValueError, SQLAlchemyError): |
| try: | ||
| with Session(engine) as session: | ||
| stats = get_snapshot_quality(session, uuid.UUID(item_id)) | ||
| except (ValueError, SQLAlchemyError): |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #232 +/- ##
==========================================
- Coverage 72.32% 71.34% -0.99%
==========================================
Files 63 63
Lines 4304 4767 +463
==========================================
+ Hits 3113 3401 +288
- Misses 1191 1366 +175 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR refactors how alignment quality statistics are represented and surfaced across the codebase by moving aggregated quality stats into the models “readers” layer and reworking the TUI to show inline quality panels and per-entity notes.
Changes:
- Introduces
SeismogramQualityStatsas a read model (event/station/snapshot aggregation) and adds new core helpers to fetch/dump those stats. - Adds a new
AimbatNotetable plus CLI/TUI support for reading/editing notes on events/stations/seismograms/snapshots. - Updates the TUI layout (quality side panels, seismogram plot widget, help modal) and adjusts docs accordingly; removes the old
core/_quality.pymodule.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds textual-plotext (+ transitive plotext) and bumps some locked deps. |
| pyproject.toml | Declares textual-plotext runtime dependency. |
| src/aimbat/models/_readers.py | Adds SeismogramQualityStats read model and aggregation constructors; updates some type hints. |
| src/aimbat/models/_models.py | Adds rich metadata to some IDs and introduces new AimbatNote table. |
| src/aimbat/core/_event.py | Adds event-quality retrieval + dumping and reorganises event parameter/quality dump helpers. |
| src/aimbat/core/_station.py | Adds station-quality retrieval + dumping for station quality stats. |
| src/aimbat/core/_snapshot.py | Adds snapshot-quality retrieval + dumping for snapshot quality stats. |
| src/aimbat/core/_note.py | New core module to read/write AimbatNote content. |
| src/aimbat/core/_quality.py | Deletes the previous quality/view aggregation module. |
| src/aimbat/core/init.py | Exports note APIs; stops exporting _quality. |
| src/aimbat/_tui/app.py | Major TUI layout and interaction changes: inline quality panels, note widgets, seismogram plot widget, help binding. |
| src/aimbat/_tui/_widgets.py | Adds NoteWidget and SeismogramPlotWidget; extends VimDataTable focus messaging; integrates textual_plotext. |
| src/aimbat/_tui/_format.py | Replaces the old “quality modal formatting” with format_quality_panel() for inline panels. |
| src/aimbat/_tui/modals.py | Removes QualityModal, improves snapshot details modal, adds HelpModal and markdown help loading. |
| src/aimbat/_tui/aimbat.tcss | Updates layout CSS; removes QualityModal styling; adds HelpModal styling and new panel/widget styling. |
| src/aimbat/_tui/help/tab-project.md | New/updated project-tab help content. |
| src/aimbat/_tui/help/tab-seismograms.md | New/updated live-data-tab help content. |
| src/aimbat/_tui/help/tab-snapshots.md | New/updated snapshots-tab help content. |
| src/aimbat/_cli/common/_parameters.py | Adds station “all” selection parameters and introduces open_in_editor() helper; docstring improvements. |
| src/aimbat/_cli/event.py | Adds event note and event quality subcommands. |
| src/aimbat/_cli/station.py | Adds station note and station quality subcommands. |
| src/aimbat/_cli/seismogram.py | Adds seismogram note subcommand. |
| src/aimbat/_cli/snapshot.py | Adds snapshot note and snapshot quality subcommands. |
| tests/integration/core/test_views.py | Refactors integration tests to target SeismogramQualityStats (event/station/snapshot). |
| tests/functional/test_tui.py | Expands engine monkeypatching to additional TUI modules. |
| docs/usage/index.md | Updates CLI/TUI usage narrative and examples (incl. event selection guidance). |
| docs/usage/alignment.md | Updates CLI vs shell examples formatting and event-ID usage in examples. |
| docs/usage/mccc.md | Updates CLI vs shell examples formatting and event-ID usage in examples. |
| docs/usage/inspection.md | Updates CLI vs shell examples and adds extra inspection guidance text. |
| docs/usage/snapshots.md | Updates snapshot CLI/shell examples and expands dump output description. |
| docs/usage/project.md | Splits “CLI / Shell” sections into separate “CLI” and “Shell”. |
| docs/usage/iccs-stack.md | Expands “Live data” explanation and updates wording from “CC norm” → “CC”. |
| docs/usage/event-selection.md | Updates event-selection guidance and examples. |
| docs/usage/api.md | Expands API docs with quality-analysis patterns and examples. |
| docs/development/database-models.md | Updates ER diagram fields to match current schema naming. |
| @@ -1,43 +1,59 @@ | |||
| """Integration tests for quality view functions in aimbat.core._quality.""" | |||
| """Integration tests for SeismogramQualityStats in aimbat.core._quality.""" | |||
There was a problem hiding this comment.
The module docstring says these are tests for SeismogramQualityStats in aimbat.core._quality, but the class under test is now imported from aimbat.models. Update the docstring to reference the new location to avoid misleading future readers.
| def get_note_content(session: Session, target: NoteTarget, target_id: uuid.UUID) -> str: | ||
| """Return the note content for the given entity. | ||
|
|
||
| Args: | ||
| session: Active database session. | ||
| target: Entity type — one of `event`, `station`, `seismogram`, `snapshot`. | ||
| target_id: UUID of the target entity. | ||
|
|
||
| Returns: | ||
| Markdown note content, or an empty string if no note exists yet. | ||
| """ | ||
| attr = getattr(AimbatNote, f"{target}_id") | ||
| note = session.exec(select(AimbatNote).where(attr == target_id)).first() | ||
| return note.content if note is not None else "" |
There was a problem hiding this comment.
This code assumes the aimbatnote table exists. For users with an existing project DB created before AimbatNote was added, select(AimbatNote) will raise a SQLite "no such table" error which aimbat.db converts into a misleading RuntimeError("No AIMBAT project found..."). Consider adding an explicit schema upgrade/ensure step (e.g. SQLModel.metadata.create_all(engine, tables=[AimbatNote.__table__]) on startup) or handling the missing-table case here with a clearer error/message.
| class AimbatNote(SQLModel, table=True): | ||
| """Free-text Markdown note attached to an event, station, seismogram, or snapshot. | ||
|
|
||
| At most one of the four FK fields is set per row. Deletion of the parent | ||
| record cascades to delete the note via the DB-level foreign key constraint. | ||
| """ | ||
|
|
||
| model_config = SQLModelConfig( | ||
| alias_generator=to_camel, | ||
| populate_by_name=True, | ||
| ) | ||
|
|
||
| id: uuid.UUID = Field( | ||
| default_factory=uuid.uuid4, | ||
| primary_key=True, | ||
| description="Unique note ID.", | ||
| ) | ||
| content: str = Field( | ||
| default="", | ||
| description="Note content in Markdown format.", | ||
| ) | ||
| event_id: uuid.UUID | None = Field( | ||
| default=None, | ||
| foreign_key="aimbatevent.id", | ||
| ondelete="CASCADE", | ||
| description="Foreign key referencing the parent event.", | ||
| ) | ||
| station_id: uuid.UUID | None = Field( | ||
| default=None, | ||
| foreign_key="aimbatstation.id", | ||
| ondelete="CASCADE", | ||
| description="Foreign key referencing the parent station.", | ||
| ) | ||
| seismogram_id: uuid.UUID | None = Field( | ||
| default=None, | ||
| foreign_key="aimbatseismogram.id", | ||
| ondelete="CASCADE", | ||
| description="Foreign key referencing the parent seismogram.", | ||
| ) | ||
| snapshot_id: uuid.UUID | None = Field( | ||
| default=None, | ||
| foreign_key="aimbatsnapshot.id", | ||
| ondelete="CASCADE", | ||
| description="Foreign key referencing the parent snapshot.", | ||
| ) |
There was a problem hiding this comment.
AimbatNote's docstring says "At most one of the four FK fields is set per row", but there is no DB-level constraint or model-level validation enforcing that invariant. Adding a check constraint (or a model_validator that raises if more than one target FK is non-null) would prevent ambiguous notes and make the documented behaviour true.
| def _auto_save(self) -> None: | ||
| if self._target_type is None or self._target_id is None: | ||
| return | ||
| with suppress(NoMatches): | ||
| content = self.query_one("#note-textarea", _NoteTextArea).text | ||
| if content != self._saved_content: | ||
| with Session(engine) as session: | ||
| save_note(session, self._target_type, self._target_id, content) # type: ignore[arg-type] | ||
| self._saved_content = content |
There was a problem hiding this comment.
_auto_save() suppresses NoMatches when looking up #note-textarea, but then uses content outside the with suppress(...) block. If the widget isn't mounted yet (or the subtree can't be found), this will raise UnboundLocalError and potentially crash the TUI. Initialise content before the suppress block (or return early when the query fails) so the method is safe during mount/unmount transitions.
| @@ -268,6 +281,13 @@ def on_mount(self) -> None: | |||
| self._iccs_last_modified_seen: Timestamp | None = None | |||
| self._current_event_id: uuid.UUID | None = None | |||
| self._active_tab: str = "tab-seismograms" | |||
There was a problem hiding this comment.
TabbedContent is initialised with initial="tab-project", but _active_tab is set to "tab-seismograms" on mount. This can make action_show_help() open help for the wrong tab until a TabActivated event fires. Initialise _active_tab to "tab-project" (or derive it from the TabbedContent active pane) to keep state consistent.
| self._active_tab: str = "tab-seismograms" | |
| self._active_tab: str = "tab-project" |
| event_id: UUID | None = Field( | ||
| default=None, | ||
| title="Event ID", | ||
| exclude_if=lambda v: v is None, | ||
| json_schema_extra={ | ||
| "rich": RichColSpec(style="magenta", no_wrap=True, highlight=False), # type: ignore[dict-item] | ||
| }, | ||
| ) | ||
| snapshot_id: UUID | None = Field( | ||
| default=None, | ||
| title="Snapshot ID", | ||
| exclude_if=lambda v: v is None, | ||
| json_schema_extra={ | ||
| "rich": RichColSpec(style="magenta", no_wrap=True, highlight=False), # type: ignore[dict-item] | ||
| }, | ||
| ) | ||
| station_id: UUID | None = Field( | ||
| default=None, | ||
| title="Station ID", | ||
| exclude_if=lambda v: v is None, | ||
| json_schema_extra={ | ||
| "rich": RichColSpec(style="magenta", no_wrap=True, highlight=False), # type: ignore[dict-item] | ||
| }, | ||
| ) |
There was a problem hiding this comment.
exclude_if=... on Field(...) isn't referenced anywhere else in the codebase, and Pydantic serialisation won't automatically drop fields based on that metadata. As a result, station_id / snapshot_id / event_id will likely appear as always-null columns in the quality tables unless callers manually exclude them. Consider removing exclude_if and instead: (a) pass exclude_none=True where these models are dumped, or (b) provide separate read models for event/station/snapshot quality so only relevant ID fields exist.
| editor = os.environ.get("EDITOR") or os.environ.get("VISUAL") | ||
| if not editor: | ||
| editor = "notepad" if sys.platform == "win32" else "vi" | ||
|
|
||
| with tempfile.NamedTemporaryFile( | ||
| mode="w", | ||
| suffix=".md", | ||
| delete=False, | ||
| encoding="utf-8", | ||
| ) as tmp: | ||
| tmp.write(initial_content) | ||
| tmp_path = tmp.name | ||
|
|
||
| try: | ||
| subprocess.call([*editor.split(), tmp_path]) | ||
| with open(tmp_path, encoding="utf-8") as f: | ||
| return f.read() |
There was a problem hiding this comment.
open_in_editor() builds the editor argv via editor.split(), which breaks common $EDITOR values containing quoted args or spaces (e.g. EDITOR="code --wait", paths with spaces). Use shlex.split(editor) (and consider subprocess.run(..., check=False) for clearer return-code handling) so note editing works reliably across platforms.
| for seis_id in seismogram_ids: | ||
| sq = session.exec( | ||
| select(AimbatSeismogramQuality).where( | ||
| col(AimbatSeismogramQuality.seismogram_id) == seis_id | ||
| ) | ||
| ).first() | ||
| if sq is None: | ||
| sq = AimbatSeismogramQuality(id=uuid.uuid4(), seismogram_id=seis_id) | ||
| sq.iccs_cc = 0.8 | ||
| if with_mccc: | ||
| sq.mccc_error = pd.Timedelta(microseconds=100) | ||
| sq.mccc_cc_mean = 0.9 | ||
| sq.mccc_cc_std = 0.05 | ||
| session.add(sq) | ||
| session.commit() |
There was a problem hiding this comment.
_write_seismogram_quality(..., with_mccc=False) never clears existing MCCC fields (mccc_error, mccc_cc_mean, mccc_cc_std) on already-present AimbatSeismogramQuality rows. This makes test_is_independent_of_live_quality_changes ineffective (the second write doesn't actually change live MCCC quality). When with_mccc is False, explicitly set the MCCC fields to None (or overwrite them with different values) so the test genuinely verifies snapshot isolation.
| def _auto_save(self) -> None: | ||
| if self._target_type is None or self._target_id is None: | ||
| return | ||
| content = self._saved_content |
📚 Documentation preview 📚: https://aimbat--232.org.readthedocs.build/en/232/