Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #217 +/- ##
==========================================
- Coverage 94.37% 88.04% -6.34%
==========================================
Files 45 46 +1
Lines 1707 1890 +183
==========================================
+ Hits 1611 1664 +53
- Misses 96 226 +130 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR introduces MCCC alignment support and reshapes the CLI/data-dumping pathways to return/print structured JSON and render parameter tables via Rich, alongside some dependency updates.
Changes:
- Add an MCCC runner (
run_mccc) and a newalignCLI command to run ICCS or MCCC alignment. - Refactor multiple
dump_*helpers to return JSON strings (using PydanticTypeAdapter) and update CLIs/tests accordingly. - Reorganise CLI commands (e.g.
event parameter …,seismogram parameter …, newplot …/pick …) and remove the oldiccsCLI module.
Reviewed changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updates Python dependency lock entries (e.g. greenlet, mkdocstrings-python, sqlmodel, pysmo). |
| flake.lock | Updates pinned Nixpkgs revision/hash. |
| tests/test_station.py | Updates station dump test to validate returned JSON via TypeAdapter. |
| tests/test_seismogram.py | Updates CLI subcommand paths and validates dump JSON via TypeAdapter; updates plot CLI invocation. |
| tests/test_event.py | Updates event CLI paths (parameter subcommand), and validates dump JSON via TypeAdapter. |
| tests/test_data.py | Updates data dump test to validate returned JSON via TypeAdapter. |
| src/aimbat/utils/_uuid.py | Improves UUID prefix matching via SQL and extends uuid_shortener to accept a model class + UUID string. |
| src/aimbat/utils/_style.py | Minor table styling cleanup. |
| src/aimbat/utils/_json.py | Replaces JSON dumping with json_to_table for rendering dict/list-of-dicts as Rich tables. |
| src/aimbat/utils/_active_event.py | Updates import path for CLI hints. |
| src/aimbat/utils/init.py | Reorders star-imports for utils modules. |
| src/aimbat/models/_models.py | Converts AimbatSeismogram.end_time to a Pydantic computed_field for serialisation. |
| src/aimbat/core/_station.py | Changes station dump to return JSON string (dump_station_table_to_json). |
| src/aimbat/core/_snapshot.py | Adds snapshot dump-to-JSON helper (string or python JSON). |
| src/aimbat/core/_seismogram.py | Adds seismogram dump-to-JSON and parameter dump/list helpers; uses json_to_table. |
| src/aimbat/core/_iccs.py | Adds run_mccc; renames ICCS plot function to plot_iccs_seismograms. |
| src/aimbat/core/_event.py | Adds event dump-to-JSON and parameter dump/list helpers; uses json_to_table. |
| src/aimbat/core/_data.py | Changes data dump to return JSON string (dump_data_table_to_json). |
| src/aimbat/cli/iccs.py | Removes legacy iccs CLI module (replaced by align/plot/pick). |
| src/aimbat/cli/_utils/sampledata.py | Updates imports to new CLI common module path. |
| src/aimbat/cli/_utils/app.py | Fixes relative imports for utils CLI app wiring. |
| src/aimbat/cli/_utils/init.py | Adds package init and exports app. |
| src/aimbat/cli/_station.py | Updates station dump to print returned JSON via rich.print_json. |
| src/aimbat/cli/_snapshot.py | Adds snapshot dump command and updates imports. |
| src/aimbat/cli/_seismogram.py | Adds seismogram dump, introduces parameter subcommands, and adds parameter dump/list commands. |
| src/aimbat/cli/_project.py | Updates imports to new CLI common module path. |
| src/aimbat/cli/_plot.py | Introduces new plot CLI (data/stack/image). |
| src/aimbat/cli/_pick.py | Introduces new pick CLI for interactive ICCS parameter updates. |
| src/aimbat/cli/_event.py | Introduces parameter subcommands, adds event dump, and reorders/updates commands. |
| src/aimbat/cli/_data.py | Updates data dump to print returned JSON via rich.print_json. |
| src/aimbat/cli/_common.py | Introduces PlotParameters / IccsPlotParameters dataclasses and adjusts global params. |
| src/aimbat/cli/_align.py | Adds align iccs and align mccc CLI commands. |
| src/aimbat/app.py | Rewires top-level CLI to new module names (_align, _plot, _pick, etc.). |
| src/aimbat/aimbat_types/_pydantic.py | Inlines timedelta validators and adds PlainSerializer for timedelta JSON output. |
| src/aimbat/_lib/validators.py | Removes now-redundant pandas timedelta validator helpers. |
| src/aimbat/_config.py | Expands settings help text (with minor spelling issues). |
| def print_seismogram_parameter_table(session: Session, short: bool) -> None: | ||
| """Print a pretty table with AIMBAT seismogram parameter values for the active event. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Docstring Args section omits the session parameter even though it’s required by the function signature. Please document session for consistency with other public core helpers in this module.
| Args: | |
| Args: | |
| session: Database session used to query seismogram parameters for the | |
| active event. |
| These defaults control the default behavior of AIMBAT within a project. | ||
| They can be changed using environment variables of the same name, or by | ||
| adding a `.env` file to the current working directory. | ||
| Overriding these defaults can be done on a per-project basis in the |
There was a problem hiding this comment.
Use British English spelling in docstrings: behavior → behaviour.
| if isinstance(data, dict): | ||
| key_kw = {**common_column_kwargs, **column_kwargs.get("Key", {})} | ||
| val_kw = {**common_column_kwargs, **column_kwargs.get("Value", {})} | ||
| table.add_column(key_kw.pop("header", "Key"), **key_kw) | ||
| table.add_column(val_kw.pop("header", "Value"), **val_kw) |
There was a problem hiding this comment.
common_column_kwargs is forwarded into Table.add_column(...) (via key_kw/val_kw/col_kw). Callers in this PR pass {"highlight": True}, but highlight is a Table(...) constructor argument (not an add_column kwarg) in Rich; this will raise TypeError: add_column() got an unexpected keyword argument 'highlight' at runtime. Consider either removing highlight support from common_column_kwargs/validating allowed keys, or adding a highlight: bool parameter to make_table(...)/json_to_table(...) and applying it when the Table is constructed instead of when columns are added.
| data=active_event.parameters.model_dump(mode="json"), | ||
| title=f"Event parameters for event: {uuid_shortener(session, active_event) if short else str(active_event.id)}", | ||
| skip_keys=["id", "event_id"], | ||
| common_column_kwargs={"highlight": True}, | ||
| column_kwargs={ |
There was a problem hiding this comment.
Same issue as above: common_column_kwargs={"highlight": True} will be forwarded into Table.add_column(...) by json_to_table(...), which is not a valid column kwarg in Rich and will raise at runtime. Remove it here or apply highlight at table construction time instead.
| They can be changed using environment variables of the same name, or by | ||
| adding a `.env` file to the current working directory. | ||
| Overriding these defaults can be done on a per-project basis in the | ||
| fllowing ways (in order of precedence): |
There was a problem hiding this comment.
Typo in docstring: fllowing → following.
| fllowing ways (in order of precedence): | |
| following ways (in order of precedence): |
| "event_id": lambda x: ( | ||
| uuid_shortener(session, AimbatEvent, str_uuid=x) if short else x | ||
| ), | ||
| }, | ||
| common_column_kwargs={"highlight": True}, | ||
| column_kwargs={ |
There was a problem hiding this comment.
common_column_kwargs={"highlight": True} is passed into json_to_table(...), but json_to_table currently forwards common_column_kwargs into Table.add_column(...). In Rich, highlight is a table-level option (constructor kwarg), so this will raise at runtime when listing parameters. Either drop this kwarg here, or adjust json_to_table/make_table to apply highlight when creating the table.
| title=title, | ||
| skip_keys=["id"], | ||
| column_order=["seismogram_id", "select"], | ||
| common_column_kwargs={"highlight": True}, |
There was a problem hiding this comment.
common_column_kwargs={"highlight": True} is passed to json_to_table(...), but json_to_table forwards these kwargs into Table.add_column(...). highlight is a table-level option in Rich (constructor kwarg), so this will raise at runtime when running seismogram parameter list. Remove this kwarg here or update json_to_table/make_table to handle it at table creation.
| common_column_kwargs={"highlight": True}, |
| def uuid_shortener[T: AimbatTypes]( | ||
| session: Session, | ||
| aimbat_obj: AimbatTypes, | ||
| min_length: int = settings.min_id_length, | ||
| aimbat_obj: T | type[T], | ||
| min_length: int = 2, | ||
| str_uuid: str | None = None, |
There was a problem hiding this comment.
uuid_shortener no longer uses settings.min_id_length as its default and instead hard-codes min_length=2. This means user/project configuration of min_id_length in settings will be ignored unless every call site passes min_length explicitly. Consider restoring min_length: int = settings.min_id_length (and keeping the ability to override it per-call).
| ) -> str | list[dict[str, Any]]: | ||
| """Dump the `AimbatSnapshot` table data to json.""" | ||
|
|
||
| logger.info("Dumping AimbatSeismogramtable to json.") |
There was a problem hiding this comment.
The log message refers to AimbatSeismogramtable, which looks like a copy/paste mistake and makes logs confusing when dumping snapshots. Update it to mention the snapshot table (and add a missing space if needed).
| logger.info("Dumping AimbatSeismogramtable to json.") | |
| logger.info("Dumping AimbatSnapshot table to json.") |
| def run_mccc(session: Session, iccs: ICCS, all_events: bool = False) -> None: | ||
| """Run MCCC algorithm. | ||
|
|
||
| Args: | ||
| session: Database session. | ||
| iccs: ICCS instance. | ||
| all_events: Whether to run MCCC for all events. | ||
| """ | ||
|
|
||
| logger.info(f"Running MCCC with {all_events=}.") | ||
|
|
||
| cc_seismograms = ( | ||
| iccs.cc_seismograms | ||
| if all_events | ||
| else [s for s in iccs.cc_seismograms if s.parent_seismogram.select] | ||
| ) |
There was a problem hiding this comment.
Parameter name and docstring are misleading: all_events is used to decide whether to include all seismograms vs only parent_seismogram.select, not whether to run across multiple events. Renaming to something like include_deselected/include_all_seismograms (and updating the log line) would make the API self-describing and avoid confusion with the CLI flag.
📚 Documentation preview 📚: https://aimbat--217.org.readthedocs.build/en/217/