Correct API implementations for all phases#152
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes curryer.correction APIs around configuration, verification/error-statistics, and pipeline outputs by centralizing constants, introducing typed inputs/results, and adding unified path resolution (including S3 URIs).
Changes:
- Refactors error-stats to use WGS84 constants internally and splits “per-measurement nadir-equivalent” computation from aggregate statistics; adds
compute_percent_below. - Introduces structured models (
CorrectionInput,CorrectionResult/ParameterSetResult,RequirementsConfig) and updates verification provenance fields + new verification input signatures (file-path modes stubbed asNotImplementedError). - Adds unified
resolve_path()(local + S3) and updates pipeline/image loaders to use it; adds support for direct calibration file paths.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_correction/test_verification.py | Updates verification tests for new override/provenance; adds tests for new NotImplemented input modes and pairing-summary logger (currently contains a broken import). |
| tests/test_correction/test_results.py | New test suite for structured correction/verification results and formatting utilities. |
| tests/test_correction/test_pipeline.py | Updates image matching override usage; adds coverage for position_columns extraction helper. |
| tests/test_correction/test_parameters.py | Removes earth_radius_m from config construction in tests. |
| tests/test_correction/test_kernel_ops.py | Adds coverage for direct calibration file-path behavior and missing-file errors. |
| tests/test_correction/test_io.py | New tests for resolve_path() and S3 temp download/cleanup behavior. |
| tests/test_correction/test_geolocation_error_stats.py | Removes legacy large error-stats test module (replacement appears incomplete/missing). |
| tests/test_correction/test_config.py | Updates config tests for deprecations, CorrectionInput, and JSON-loading behavior w/ deprecated earth_radius_m. |
| tests/test_correction/clarreo/test_clarreo_config.py | Updates CLARREO config tests to stop expecting earth_radius_m. |
| tests/test_correction/clarreo/clarreo_data_loaders.py | Clarifies docstring around preprocessing scripts and deprecations. |
| tests/test_correction/clarreo/clarreo_config.py | Updates docs/code to use _image_matching_override; removes earth_radius_m from generated config. |
| tests/test_correction/clarreo/_pipeline_helpers.py | Updates helpers to use _image_matching_override; removes earth_radius_m usage. |
| scripts/example_verification_minimal.py | Updates example to use _image_matching_override and removes earth_radius_m. |
| curryer/correction/verification.py | Adds provenance fields, pairing-summary logger, file-path-based verify signature (stubbed), and compare_results(). |
| curryer/correction/results.py | Adds structured CorrectionResult/ParameterSetResult and summary-table builder + factory. |
| curryer/correction/pipeline.py | Adds run_correction() API, position_columns extraction, calibration direct-path support, S3-aware file loading, and revised per-pair error computation. |
| curryer/correction/io.py | New unified path resolver with S3 download-to-temp support + atexit cleanup. |
| curryer/correction/image_io.py | Updates MAT loaders to accept `str |
| curryer/correction/error_stats.py | Centralizes Earth radius, adds compute_percent_below(), adds “nadir-only” compute method, and makes stats mission-agnostic (no pass/fail). |
| curryer/correction/config.py | Removes earth_radius_m usage, adds RequirementsConfig, deprecates image_matching_func in favor of _image_matching_override, adds position_columns, adds CorrectionInput. |
| curryer/correction/init.py | Re-exports new APIs and utilities (also exports an underscore-prefixed helper). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0a09d1c to
d6bb926
Compare
6638b56 to
ae447ac
Compare
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (86.87%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## 122-refactor-correction-package-epic #152 +/- ##
========================================================================
+ Coverage 76.52% 77.93% +1.40%
========================================================================
Files 90 94 +4
Lines 12303 13145 +842
Branches 1331 1370 +39
========================================================================
+ Hits 9415 10244 +829
- Misses 2388 2398 +10
- Partials 500 503 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… _log_pairing_summary from package exports Agent-Logs-Url: https://github.com/lasp/curryer/sessions/928b19d7-06dc-4719-b7a7-938ba4030f6d Co-authored-by: mmaclay <21048535+mmaclay@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/lasp/curryer/sessions/5c4dcff0-2344-472a-9c8d-1f05116a0ce9 Co-authored-by: mmaclay <21048535+mmaclay@users.noreply.github.com>
771c7ca
into
122-refactor-correction-package-epic
This pull request introduces several improvements and refactors to the
curryer.correctionpackage, focusing on configuration, error statistics processing, and package exports. The changes modernize configuration handling (notably removing the need for a user-supplied Earth radius), clarify the API for error statistics and correction inputs, and enhance maintainability by deprecating legacy mechanisms. The most important changes are grouped below.Configuration and API Improvements
earth_radius_minCorrectionConfig; the WGS84 Earth radius fromcurryer.compute.constantsis now used as the single source of truth. The field is deprecated and ignored if present in config files. [1] [2] [3] [4] [5]RequirementsConfigclass for specifying verification thresholds, which can be attached toCorrectionConfigor passed directly to verification routines. [1] [2] [3] [4]CorrectionInput, a typed structure for correction loop inputs, replacing positional tuples with named fields for clarity and IDE support. [1] [2]psf_file,los_vectors_file) inCorrectionConfigas an alternative to directory+filename patterns.image_matching_funcpublic property in favor of a private attribute_image_matching_overridefor test injection, with deprecation warnings.Error Statistics Refactoring
curryer.compute.constantsthroughout. [1] [2]compute_percent_below, a utility function for computing the percentage of errors below a custom threshold, and clarified that pass/fail logic is now the caller's responsibility. [1] [2] [3] [4]ErrorStatsConfigandErrorStatsProcessorto no longer require performance thresholds or Earth radius, and improved documentation to clarify their mission-agnostic role. [1] [2]Package Structure and Exports
__init__.pyto re-export new and refactored classes/functions, includingCorrectionInput,RequirementsConfig,compute_percent_below, calibration result types, and new pipeline entry points. [1] [2] [3] [4] [5]Data Configuration
position_columnstoDataConfigfor explicit telemetry spacecraft-position column mappings, improving flexibility for different data sources.These changes collectively modernize the configuration and error statistics APIs, clarify the separation of concerns between statistics and verification, and improve usability for downstream users and developers.
[Copilot is generating a summary...]