refactor(core): re-arange core, move set_default_event and friends ou…#224
Merged
refactor(core): re-arange core, move set_default_event and friends ou…#224
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| def get_event_parameter(session: Session, name: EventParameterBool) -> bool: ... | ||
| def get_event_parameter( | ||
| session: Session, event: AimbatEvent, name: EventParameterBool | ||
| ) -> bool: ... |
| def get_event_parameter(session: Session, name: EventParameterFloat) -> float: ... | ||
| def get_event_parameter( | ||
| session: Session, event: AimbatEvent, name: EventParameterFloat | ||
| ) -> float: ... |
| def plot_all_seismograms(session: Session, return_fig: Literal[False]) -> None: ... | ||
| def plot_all_seismograms( | ||
| session: Session, event: AimbatEvent, return_fig: Literal[False] | ||
| ) -> None: ... |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
==========================================
- Coverage 72.29% 70.33% -1.97%
==========================================
Files 50 50
Lines 2805 2869 +64
==========================================
- Hits 2028 2018 -10
- Misses 777 851 +74 ☔ View full report in Codecov by Sentry. |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors aimbat.core to reduce implicit “active event” coupling by requiring explicit event arguments for many operations, while moving table-printing responsibilities into CLI commands and extending the TUI to detect/handle ICCS staleness via an event.last_modified timestamp.
Changes:
- Update core APIs (snapshots/events/seismograms/stations/data/ICCS) to accept an explicit
AimbatEvent(orevent=) instead of always resolving the active event internally. - Add
AimbatEvent.last_modifiedplus SQLite triggers to update it when parameters change; introduceBoundICCSand staleness checks in the TUI. - Refresh CLI implementations and documentation/navigation to reflect the new interface structure (CLI/TUI/GUI/API/Shell).
Reviewed changes
Copilot reviewed 46 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| zensical.toml | Reorganises docs navigation (CLI/Shell/TUI/GUI/API/Defaults). |
| uv.lock | Locks new dependency additions (prompt-toolkit + wcwidth). |
| pyproject.toml | Adds prompt-toolkit dependency. |
| .gitignore | Ignores scratch/ directory. |
| tests/integration/models/test_operations.py | Updates snapshot calls to pass explicit event. |
| tests/integration/models/test_models.py | Updates snapshot creation to use explicit active event. |
| tests/integration/core/test_station.py | Updates station APIs; removes printing tests. |
| tests/integration/core/test_snapshots.py | Updates snapshot APIs; removes printing tests. |
| tests/integration/core/test_seismogram.py | Updates seismogram APIs; removes printing tests. |
| tests/integration/core/test_project.py | Removes print_project_info tests; adjusts delete behaviour. |
| tests/integration/core/test_event.py | Updates event parameter APIs; removes printing tests. |
| tests/integration/core/test_data.py | Updates data APIs; removes printing tests. |
| src/aimbat/utils/_style.py | Handles NaT timestamps in table formatting. |
| src/aimbat/models/_readers.py | Adds last_modified to event read model. |
| src/aimbat/models/_parameters.py | Import ordering only (no functional change). |
| src/aimbat/models/_models.py | Adds AimbatEvent.last_modified column. |
| src/aimbat/models/init.py | Re-exports parameter base classes. |
| src/aimbat/core/_station.py | Renames/refactors station queries; adds JSON dump with counts. |
| src/aimbat/core/_snapshot.py | Requires explicit event for snapshot creation and filtering. |
| src/aimbat/core/_seismogram.py | Requires explicit event for selection/parameter dumping and plotting. |
| src/aimbat/core/_project.py | Adds SQLite triggers to update last_modified; removes print helper. |
| src/aimbat/core/_iccs.py | Introduces BoundICCS; makes ICCS ops event-explicit. |
| src/aimbat/core/_event.py | Makes event parameter APIs event-explicit; trims printing helpers. |
| src/aimbat/core/_data.py | Makes data queries event-explicit; removes printing helper. |
| src/aimbat/core/_active_event.py | Import reordering to avoid circular imports. |
| src/aimbat/_types/_seismogram.py | Updates doc links to new module paths. |
| src/aimbat/_types/_event.py | Updates doc links to new module paths. |
| src/aimbat/_tui/app.py | Uses BoundICCS + periodic staleness detection; shows modified time. |
| src/aimbat/_cli/utils/sampledata.py | Aligns global parameter handling with other CLI commands. |
| src/aimbat/_cli/station.py | Rebuilds station list output using core JSON/dump helpers. |
| src/aimbat/_cli/snapshot.py | Rebuilds snapshot list/details output using JSON dump helpers. |
| src/aimbat/_cli/seismogram.py | Rebuilds seismogram list/parameter list around updated core APIs. |
| src/aimbat/_cli/project.py | Inlines former project info printing into CLI command. |
| src/aimbat/_cli/plot.py | Adapts plotting commands to pass explicit event/ICCS instance. |
| src/aimbat/_cli/pick.py | Adapts pick tools to explicit event/ICCS instance. |
| src/aimbat/_cli/event.py | Rebuilds event list/parameter list around JSON table output. |
| src/aimbat/_cli/data.py | Rebuilds data list output in CLI (moved from core). |
| src/aimbat/_cli/align.py | Adapts ICCS/MCCC run commands to explicit event/ICCS instance. |
| docs/usage/tui.md | Adds TUI usage page stub. |
| docs/usage/shell.md | Adds shell usage page stub. |
| docs/usage/index.md | Updates usage overview and interface descriptions. |
| docs/usage/gui.md | Updates GUI heading. |
| docs/usage/defaults.md | Updates defaults heading. |
| docs/usage/cli.md | Replaces CLI page with new overview + validation note. |
| docs/usage/api.md | Adds Python API overview page. |
| docs/first-steps/workflow.md | Rewrites workflow content; adds proper citations and guidance. |
| docs/first-steps/installation.md | Rewrites installation instructions (uv/pipx links, examples). |
| docs/first-steps/data.md | Rewrites data model explanation and improves formatting. |
Comment on lines
+68
to
71
| ## Installing locally | ||
|
|
||
| After successfully test driving AIMBAT you may want to actually install it. | ||
| This is very simple with `uv`: | ||
| Permanent installation of is just as easy. Just run `uv tool install`: | ||
|
|
There was a problem hiding this comment.
The sentence “Permanent installation of is just as easy.” is missing the object (e.g. “Permanent installation of AIMBAT…”). This reads as a typo in the rendered docs.
Comment on lines
+50
to
+54
| with Session(engine) as session: | ||
| print_json(dump_station_table_to_json(session)) | ||
|
|
||
| if __name__ == "__main__": | ||
| app() |
There was a problem hiding this comment.
if __name__ == "__main__": app() is currently indented inside cli_station_dump(), which means it only runs when that command executes and the module has no top-level entrypoint when run as a script. Move the main guard back to module scope (bottom of file) and remove it from inside the command function.
Comment on lines
+98
to
+155
| # @overload | ||
| # def get_stations_with_event_seismogram_count( | ||
| # session: Session, as_json: Literal[False] | ||
| # ) -> Sequence[tuple[AimbatStation, int, int]]: ... | ||
| # | ||
| # | ||
| # @overload | ||
| # def get_stations_with_event_seismogram_count( | ||
| # session: Session, as_json: Literal[True] | ||
| # ) -> list[dict[str, Any]]: ... | ||
| # | ||
| # | ||
| # def get_stations_with_event_seismogram_count( | ||
| # session: Session, as_json: bool | ||
| # ) -> Sequence[tuple[AimbatStation, int, int]] | list[dict[str, Any]]: | ||
| # """Get stations along with the count of seismograms and events they are associated with. | ||
| # | ||
| # Args: | ||
| # session: Database session. | ||
| # as_json: Whether to return the result as JSON. | ||
| # | ||
| # Returns: A sequence of tuples containing the station, count of seismograms | ||
| # and count of events, or a JSON string if as_json is True. | ||
| # """ | ||
| # logger.info("Getting stations with associated seismogram and event counts.") | ||
| # | ||
| # statement = ( | ||
| # select( | ||
| # AimbatStation, | ||
| # func.count(col(AimbatSeismogram.id)), | ||
| # func.count(func.distinct(col(AimbatEvent.id))), | ||
| # ) | ||
| # .select_from(AimbatStation) | ||
| # .join(AimbatSeismogram, isouter=True) | ||
| # .join(AimbatEvent, isouter=True) | ||
| # .group_by(col(AimbatStation.id)) | ||
| # ) | ||
| # | ||
| # logger.debug(f"Executing query: {statement}") | ||
| # results = session.exec(statement).all() | ||
| # | ||
| # if not as_json: | ||
| # return results | ||
| # | ||
| # formatted_results = [] | ||
| # | ||
| # for row in results: | ||
| # # 1. Dump the station to a dict. mode="json" safely converts UUIDs/Datetimes to strings! | ||
| # station_dict = row[0].model_dump(mode="json") | ||
| # | ||
| # # 2. Add the counts directly to the dictionary | ||
| # station_dict["seismogram_count"] = row[1] | ||
| # station_dict["event_count"] = row[2] | ||
| # | ||
| # # 3. Add to our final list | ||
| # formatted_results.append(station_dict) | ||
| # | ||
| # return formatted_results |
There was a problem hiding this comment.
Large blocks of dead code have been left commented-out (the old get_stations_with_event_seismogram_count implementation). This makes the module harder to maintain and review, and risks diverging behaviour if it’s later partially re-enabled. Delete the commented-out implementation (it’s preserved in git history) and keep only the current API.
Comment on lines
+81
to
+102
| # Trigger 3: Track last modification time when event parameters change | ||
| connection.execute(text(""" | ||
| CREATE TRIGGER IF NOT EXISTS event_modified_on_params_update | ||
| AFTER UPDATE ON aimbateventparameters | ||
| BEGIN | ||
| UPDATE aimbatevent SET last_modified = datetime('now') | ||
| WHERE id = NEW.event_id; | ||
| END; | ||
| """)) | ||
|
|
||
| # Trigger 4: Track last modification time when seismogram parameters change | ||
| connection.execute(text(""" | ||
| CREATE TRIGGER IF NOT EXISTS event_modified_on_seis_params_update | ||
| AFTER UPDATE ON aimbatseismogramparameters | ||
| BEGIN | ||
| UPDATE aimbatevent SET last_modified = datetime('now') | ||
| WHERE id = ( | ||
| SELECT event_id FROM aimbatseismogram | ||
| WHERE id = NEW.seismogram_id | ||
| ); | ||
| END; | ||
| """)) |
There was a problem hiding this comment.
last_modified is set via SQLite datetime('now'), which only has 1-second resolution. Since BoundICCS.created_at is a high-resolution pandas Timestamp, updates occurring within the same second as ICCS creation may not be detected as stale (last_modified may not compare greater than created_at). Consider storing sub-second precision in the trigger (e.g. using strftime with %f) or normalising created_at to the same resolution.
|
|
||
| Once [installed](../first-steps/installation.md), AIMBAT can be used in several ways: | ||
| Once [installed](../first-steps/installation.md), AIMBAT can be used in several | ||
| ways, each of which have unique strengths depending on the task at hand: |
There was a problem hiding this comment.
Grammar: “each of which have unique strengths” should be “each of which has unique strengths” (singular ‘each’).
Suggested change
| ways, each of which have unique strengths depending on the task at hand: | |
| ways, each of which has unique strengths depending on the task at hand: |
smlloyd
force-pushed
the
rearrange-core
branch
2 times, most recently
from
d491777 to
c41e64d
Compare
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
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.
…t of core.
📚 Documentation preview 📚: https://aimbat--224.org.readthedocs.build/en/224/