refactor: improve docstrings, io DI pattern, and data source terminology#221
refactor: improve docstrings, io DI pattern, and data source terminology#221
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #221 +/- ##
==========================================
- Coverage 93.61% 93.39% -0.22%
==========================================
Files 45 45
Lines 1879 1893 +14
==========================================
+ Hits 1759 1768 +9
- Misses 120 125 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR refactors AIMBAT’s documentation and configuration, introduces a capability-based I/O registry/DI pattern for data sources, standardises “data source” terminology, and reorganises some shared ORM/type code (notably parameter base classes and SQLAlchemy type decorators).
Changes:
- Add/expand module and API docstrings across core packages; improve SQLModel
Field(..., description=...)metadata. - Refactor
aimbat.iodispatch to per-capability registries withregister_*andsupports_*helpers; SAC registers capabilities on import. - Extract parameter base classes to
models/_parameters.pyand moveSAPandas*SQLAlchemy types intoaimbat_types.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| zensical.toml | Enables griffe_fieldz extension with include_inherited. |
| uv.lock | Adds griffe-fieldz + deps; updates several locked versions. |
| pyproject.toml | Adds griffe-fieldz to docs extras. |
| src/aimbat/aimbat_types/_sqlalchemy.py | New home for SAPandasTimestamp / SAPandasTimedelta type decorators. |
| src/aimbat/aimbat_types/init.py | Re-exports SQLAlchemy decorators from aimbat_types. |
| tests/unit/aimbat_types/test_sqlalchemy.py | Updates imports for moved SAPandas* types. |
| src/aimbat/models/_parameters.py | New extracted base classes for event/seismogram parameter fields + validation. |
| src/aimbat/models/_models.py | Uses extracted parameter bases; adds Field(description=...); “data source” terminology updates. |
| src/aimbat/models/init.py | Expanded module docstring describing ORM tables/relationships. |
| tests/integration/test_models.py | Updates imports to match extracted parameter bases. |
| src/aimbat/io/_base.py | Replaces dict dispatch with registries, registration functions, and supports_* helpers. |
| src/aimbat/io/_sac.py | Registers SAC capabilities with the new I/O registry. |
| src/aimbat/io/init.py | Imports _sac to trigger capability registration. |
| src/aimbat/core/_data.py | Uses supports_* checks; renames “Filename” to “Source”; improves logging/terminology. |
| src/aimbat/core/_event.py | Switches to internal read models; updates JSON dumping accordingly. |
| src/aimbat/core/_snapshot.py | Switches to internal read models; updates JSON dumping accordingly. |
| src/aimbat/core/_seismogram.py | Updates parameter base import path. |
| src/aimbat/core/_iccs.py | Changes run_mccc signature and passes new args into mccc(). |
| src/aimbat/core/init.py | Adds higher-level package docstring describing core responsibilities. |
| src/aimbat/_config.py | Alphabetises/cleans settings; adds MCCC-related settings; removes validator mixin. |
| src/aimbat/_lib/_mixins.py | Deletes EventParametersValidatorMixin (validation moved elsewhere). |
| src/aimbat/logger.py | Expanded logging configuration docstring. |
| src/aimbat/db.py | Expanded DB engine/module docstring; minor docstring wording. |
| CHANGELOG.md | Adds new changelog bullets (contains a typo). |
| .gitignore | Broadens ignore pattern for test log files. |
Comments suppressed due to low confidence (1)
CHANGELOG.md:35
- Typo in changelog entry: “Re-arange api reference” should be “Re-arrange API reference”.
- Switch to zensical
- Re-arange api reference
| logger.info(f"Running MCCC with {all_seismograms=}.") | ||
|
|
||
| active_event = get_active_event(session) | ||
| min_cc = active_event.parameters.min_ccnorm |
There was a problem hiding this comment.
run_mccc() reads min_cc from active_event.parameters.min_ccnorm, but the model now has a dedicated mccc_min_ccnorm parameter. Using min_ccnorm here couples MCCC behaviour to the ICCS/auto-selection threshold and ignores the MCCC-specific setting.
Use active_event.parameters.mccc_min_ccnorm (and keep min_ccnorm for ICCS/autoselect) when calling mccc().
| min_cc = active_event.parameters.min_ccnorm | |
| min_cc = active_event.parameters.mccc_min_ccnorm |
| def data(self) -> np.ndarray: | ||
| """Seismogram waveform data array.""" | ||
| if self.datasource is None: | ||
| raise ValueError("Expected a valid datasource name, got None.") | ||
| return read_seismogram_data( |
There was a problem hiding this comment.
The AimbatSeismogram.datasource relationship is treated as nullable (explicit None check), but it is annotated as non-optional and the raised error message still uses the old “datasource” terminology.
Consider changing the annotation to AimbatDataSource | None (to match the runtime check) and update the error message to use “data source” and describe the missing relationship rather than a “name”.
| "Station and event data must be imported separately before " | ||
| "adding seismogram-only data sources." |
There was a problem hiding this comment.
This NotImplementedError suggests “Station and event data must be imported separately…”, but the current CLI/core API only provides data add for ingestion (no separate station/event import path). As written, the error message gives users an action they can’t take.
Suggest rewording to only state the actual requirement (the selected datatype must support station/event/seismogram creation), and/or point to the supported data types or commands that actually exist.
| "Station and event data must be imported separately before " | |
| "adding seismogram-only data sources." | |
| "Use a data type that supports station, event and seismogram " | |
| "creation with 'aimbat data add', or extend the I/O backend to " | |
| "implement the missing operations." |
| ) | ||
|
|
||
| bandpass_fmin: float = Field( | ||
| default=0.05, |
There was a problem hiding this comment.
bandpass_fmin no longer has a non-negative constraint. Since this is a frequency lower bound, allowing negative values makes the settings object accept invalid configuration and defers the failure until later model instantiation.
Re-add ge=0 (consistent with the event-parameter model) so invalid settings are rejected early.
| default=0.05, | |
| default=0.05, | |
| ge=0, |
| # 1. Handle db_url dependency on project | ||
| """Set defaults that depend on other fields.""" | ||
| if self.db_url == "": | ||
| self.db_url = f"sqlite+pysqlite:///{self.project}" |
There was a problem hiding this comment.
With EventParametersValidatorMixin removed, Settings no longer validates that bandpass_fmax > bandpass_fmin. This means an invalid configuration can be loaded successfully and only fails later when creating AimbatEventParameters, which is harder to diagnose.
Add a model_validator (or field validator) on Settings to enforce bandpass_fmax > bandpass_fmin (ideally conditional on bandpass_apply).
| self.db_url = f"sqlite+pysqlite:///{self.project}" | |
| self.db_url = f"sqlite+pysqlite:///{self.project}" | |
| if self.bandpass_apply and self.bandpass_fmax <= self.bandpass_fmin: | |
| msg = ( | |
| "Invalid bandpass configuration: bandpass_fmax must be greater than " | |
| "bandpass_fmin when bandpass_apply is True." | |
| ) | |
| raise ValueError(msg) |
- Add module docstrings to aimbat.core, aimbat.models, aimbat.io, aimbat.db, and aimbat.logger - Add description= to all SQLModel Field() calls in models; extract parameter base classes to models/_parameters.py - Sort Settings fields alphabetically and clean up _config.py - Fix griffe_fieldz include_inherited in global zensical.toml config - Refactor io/_base.py: replace dispatch dicts with per-capability registries and registration functions; add supports_*() checks - Replace "file" terminology with "data source" throughout - Move SAPandas* types to aimbat_types/_sqlalchemy.py
📚 Documentation preview 📚: https://aimbat--221.org.readthedocs.build/en/221/