feat: implement interactive shell and major documentation update for v2#226
feat: implement interactive shell and major documentation update for v2#226
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import asyncio | ||
|
|
||
| import aimbat.db | ||
| import aimbat._tui.app |
| print(f"{event.time} {len(event.seismograms)} seismograms") | ||
|
|
||
| # Filter — seismograms marked as selected | ||
| selected = session.exec( |
There was a problem hiding this comment.
Pull request overview
This PR implements the aimbat shell interactive command (a REPL with tab-completion, command history, and live ICCS status), refactors the get_default_event() API to return None instead of raising on empty DB, improves ICCS caching (process-level cache + read-only array flag), adds ICCS validation before parameter writes, and substantially rewrites the documentation (README, API docs, shell docs, and doc snippets). Functional tests for both the shell and the TUI are also added.
Changes:
get_default_event()signature changed toAimbatEvent | None; all call sites updated withassertguards or None checks- New
aimbat shellREPL with tab-completion, event-context switching, and ICCS caching - Documentation rewrite (README,
docs/usage/api.md,docs/usage/shell.md,docs/usage/cli.md) with new API snippet files
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/aimbat/core/_default_event.py |
Changed get_default_event() to return None instead of raising; resolve_event() now raises NoResultFound when no default exists |
src/aimbat/core/_iccs.py |
Added process-level ICCS cache, clear_iccs_cache(), validate_iccs_construction(); arrays marked read-only; removed clone_to_mini |
src/aimbat/io/_base.py |
Cache now returns read-only NumPy arrays |
src/aimbat/core/_event.py |
set_event_parameter() gains validate_iccs keyword arg |
src/aimbat/_cli/shell.py |
New interactive REPL shell command |
src/aimbat/_cli/common.py |
Removed CliHints/HINTS |
src/aimbat/_tui/app.py |
Updated to use resolve_event(), simplified ICCS lifecycle |
src/aimbat/_tui/modals.py |
Updated for get_default_event() returning None |
tests/functional/test_shell.py |
New functional tests for the shell REPL |
tests/functional/test_tui.py |
New functional TUI tests via headless Textual pilot |
tests/integration/core/test_*.py |
Added assert default_event is not None guards |
tests/unit/_cli/test_common.py |
Removed TestCliHints class |
pyproject.toml / uv.lock |
Added pytest-asyncio, updated ruff, platformdirs |
README.md / docs/ |
Major documentation rewrite |
CHANGELOG.md |
New entries for this release |
zensical.toml |
Updated navigation labels |
src/aimbat/app.py |
Registered shell subcommand |
| def validate_iccs_construction(session: Session, event: AimbatEvent) -> None: | ||
| """Try to construct an ICCS instance for the event without caching the result. | ||
|
|
||
| Use this to check whether the event's current (possibly uncommitted) parameters | ||
| are compatible with ICCS construction before persisting them to the database. | ||
|
|
||
| Args: | ||
| session: Database session. | ||
| event: AimbatEvent. | ||
|
|
||
| Raises: | ||
| Any exception raised by ICCS construction (e.g. invalid parameter values). |
There was a problem hiding this comment.
The validate_iccs_construction function accepts a session: Session parameter but does not use it at all — all work is done with event.seismograms and event.parameters that were already loaded. The unused parameter adds noise to the public API and may confuse callers. Since the function is exported in __all__ it forms part of the public API. Consider either removing the session parameter or making the seismogram lazy-loading explicit so the parameter is genuinely needed.
| def validate_iccs_construction(session: Session, event: AimbatEvent) -> None: | |
| """Try to construct an ICCS instance for the event without caching the result. | |
| Use this to check whether the event's current (possibly uncommitted) parameters | |
| are compatible with ICCS construction before persisting them to the database. | |
| Args: | |
| session: Database session. | |
| event: AimbatEvent. | |
| Raises: | |
| Any exception raised by ICCS construction (e.g. invalid parameter values). | |
| def validate_iccs_construction(_session: Session, event: AimbatEvent) -> None: | |
| """Try to construct an ICCS instance for the event without caching the result. | |
| Use this to check whether the event's current (possibly uncommitted) parameters | |
| are compatible with ICCS construction before persisting them to the database. | |
| Args: | |
| event: AimbatEvent to validate for ICCS construction. | |
| Raises: | |
| Any exception raised by ICCS construction (e.g. invalid parameter values). | |
| Notes: | |
| The ``_session`` argument is accepted for API compatibility and is not used. |
| seismograms = [ | ||
| MiniICCSSeismogram( | ||
| begin_time=seis.begin_time, | ||
| delta=seis.delta, | ||
| data=seis.data, | ||
| t0=seis.t0, | ||
| t1=seis.t1, | ||
| flip=seis.flip, | ||
| select=seis.select, | ||
| extra={"id": seis.id}, | ||
| ) | ||
| for seis in event.seismograms | ||
| ] | ||
| ICCS( | ||
| seismograms=seismograms, | ||
| window_pre=p.window_pre, | ||
| window_post=p.window_post, | ||
| bandpass_apply=p.bandpass_apply, | ||
| bandpass_fmin=p.bandpass_fmin, | ||
| bandpass_fmax=p.bandpass_fmax, | ||
| min_ccnorm=p.min_ccnorm, | ||
| context_width=settings.context_width, | ||
| ) |
There was a problem hiding this comment.
The MiniICCSSeismogram construction block in validate_iccs_construction (lines 148–160) is an exact duplicate of the same block in create_iccs_instance (lines 105–117). This duplication increases maintenance risk: any future change to the construction arguments (e.g. a new field on MiniICCSSeismogram) must be applied in two places. Consider extracting the list comprehension into a shared private helper such as _build_mini_seismograms(event).
| event: AimbatEvent. | ||
|
|
||
| Raises: | ||
| Any exception raised by ICCS construction (e.g. invalid parameter values). |
There was a problem hiding this comment.
The Raises section in the validate_iccs_construction docstring says "Any exception raised by ICCS construction (e.g. invalid parameter values)." — this is a placeholder rather than a specific exception type, which violates the Google-style docstring convention used throughout the codebase (which lists concrete exception types). At minimum, the most likely exceptions (e.g. ValueError, pysmo configuration errors) should be listed, or the phrasing should follow the convention used elsewhere.
| Any exception raised by ICCS construction (e.g. invalid parameter values). | |
| ValueError: If ICCS construction fails due to invalid parameter values. | |
| Exception: If ICCS construction fails for any other reason, such as | |
| underlying pysmo configuration or runtime errors. |
| - Move aimbat source to src directory | ||
| - Use pandas Timestamp and Timedelta | ||
| - Improve docstrings, io DI pattern, and data source terminology | ||
| - **(core)** Re-arange core, move set_default_event and friends out of core. |
There was a problem hiding this comment.
The changelog entry on line 22 contains a spelling error: "didn't" should read "didn't" (curly apostrophe vs straight apostrophe is acceptable, but "did't" is not present — the actual text "didn't do anything" is fine). However, line 88 contains "Re-arange" which should be "Re-arrange".
| - **(core)** Re-arange core, move set_default_event and friends out of core. | |
| - **(core)** Re-arrange core, move set_default_event and friends out of core. |
| [dependency-groups] | ||
| test = [ | ||
| "pytest>=8.4.2", | ||
| "pytest-asyncio>=1.3.0", |
There was a problem hiding this comment.
pytest-asyncio was added to both pyproject.toml and uv.lock as a test dependency, but no tests in the repository use pytest.mark.asyncio or declare async def test_* functions. All async test code in test_tui.py uses asyncio.run() directly and does not require pytest-asyncio. This is an unnecessary dependency.
| "pytest-asyncio>=1.3.0", |
|
|
||
| # Check ICCS for whichever event was actually targeted by the command. | ||
| effective_flag = _extract_event_flag(tokens) | ||
| check_event_id = uuid.UUID(effective_flag) if effective_flag else shell_event_id |
There was a problem hiding this comment.
On line 233, effective_flag is the raw string extracted by _extract_event_flag(), which may be a short UUID prefix (e.g. "abc12345") rather than a full UUID string. Calling uuid.UUID(effective_flag) on a short prefix will raise a ValueError that is not caught here, causing the shell to crash mid-loop instead of recovering gracefully. The _parse_event_id() helper (defined in the same file at line 58) already handles prefix resolution and should be used here instead of uuid.UUID() directly.
| check_event_id = uuid.UUID(effective_flag) if effective_flag else shell_event_id | |
| try: | |
| check_event_id = ( | |
| _parse_event_id(effective_flag) if effective_flag else shell_event_id | |
| ) | |
| except Exception as exc: | |
| console.print(f"[red]{type(exc).__name__}:[/red] {exc}") | |
| check_event_id = shell_event_id |
smlloyd
force-pushed
the
feat-shell
branch
2 times, most recently
from
e199497 to
d57af1d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 69.14% 73.20% +4.05%
==========================================
Files 50 51 +1
Lines 2940 3064 +124
==========================================
+ Hits 2033 2243 +210
+ Misses 907 821 -86 ☔ View full report in Codecov by Sentry. |
smlloyd
force-pushed
the
feat-shell
branch
2 times, most recently
from
892c1c7 to
ef64d3c
Compare
- Implement 'aimbat shell' providing an interactive environment for project management - Rewrite README.md to emphasize v2 goals, features, and modern tech stack - Update documentation navigation and add comprehensive API usage snippets - Refine CLI and TUI logic for default event handling - Add functional tests for shell and TUI interfaces - Update dependencies (adding pytest-asyncio) and improve test assertions
📚 Documentation preview 📚: https://aimbat--226.org.readthedocs.build/en/226/