Skip to content

Refactor correction package tests and add integration and unit tests for CLARREO#146

Merged
mmaclay merged 18 commits into122-refactor-correction-package-epicfrom
130-pr-41-restructure-test_correction-for-maintainability
Apr 3, 2026
Merged

Refactor correction package tests and add integration and unit tests for CLARREO#146
mmaclay merged 18 commits into122-refactor-correction-package-epicfrom
130-pr-41-restructure-test_correction-for-maintainability

Conversation

@mmaclay
Copy link
Copy Markdown
Collaborator

@mmaclay mmaclay commented Apr 3, 2026

This pull request introduces new infrastructure to support integration and correction testing, especially for the CLARREO workflow. It adds synthetic data generators and helpers for running image-matching tests, as well as utilities for applying artificial geolocation errors and test parameter variations. These additions are designed to make upstream pipeline and integration tests more robust and independent of real instrument data.

Test infrastructure and synthetic data generation:

  • Added tests/test_correction/_synthetic_helpers.py with a suite of functions for generating synthetic sensor data (boresight vectors, spacecraft positions, transformation matrices, GCP pairs) and synthetic image-matching results for use in upstream and e2e tests. This enables testing without real instrument data.
  • Added tests/integration/__init__.py as a package marker for integration tests.

CLARREO image-matching test helpers:

  • Added tests/test_correction/clarreo/_image_match_helpers.py, providing utilities to discover test cases, apply artificial geolocation errors, and run image-matching with error injection. Includes parameterized test case discovery, error variation, and result caching.
  • Added tests/test_correction/clarreo/__init__.py as a package marker for CLARREO-specific correction tests.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restructures the curryer.correction test suite to support CLARREO-focused integration/e2e testing without requiring real instrument data. It replaces a large monolithic test module with smaller unit tests plus reusable helpers for synthetic data generation and downstream image-match workflows.

Changes:

  • Added unit tests for core correction pipeline helpers (NetCDF structure, parameter/metric extraction & storage, kernel ops).
  • Added CLARREO integration/e2e test helpers (test-case discovery, error injection/variation, upstream/downstream pipeline runners) plus CLARREO-specific tests.
  • Refactored existing tests from unittest style to pytest style, and split AWS-requiring S3 tests into a dedicated (skipped-by-default) integration test.

Reviewed changes

Copilot reviewed 16 out of 19 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tests/test_correction/test_results_io.py New unit tests for NetCDF structure and checkpoint save/load/cleanup.
tests/test_correction/test_pipeline.py New unit tests for key pipeline helper functions + an extra loop smoke test.
tests/test_correction/test_pairing.py Converted pairing tests from unittest to pytest; added small synthetic pairing cases.
tests/test_correction/test_kernel_ops.py New unit tests for apply_offset, calibration loading, and dynamic kernel creation (extra).
tests/test_correction/test_image_match.py Converted integrated image match tests from unittest to pytest.
tests/test_correction/test_dataio.py Converted to pytest; kept credential-free tests and moved AWS tests out.
tests/test_correction/test_correction.py Deleted large unified test harness in favor of smaller focused tests/helpers.
tests/test_correction/conftest.py Simplified fixtures and added sys.path setup for CLARREO helper imports.
tests/test_correction/clarreo/test_clarreo_e2e.py New CLARREO end-to-end/integration test coverage using helpers.
tests/test_correction/clarreo/test_clarreo_dataio.py New CLARREO S3 integration tests gated on AWS credentials.
tests/test_correction/clarreo/test_clarreo_config.py New tests validating CLARREO config generation + JSON round-trip.
tests/test_correction/clarreo/conftest.py New CLARREO path fixtures (session-scoped).
tests/test_correction/clarreo/clarreo_data_loaders.py Added CLARREO CSV preprocessing shim for tests (compatibility layer).
tests/test_correction/clarreo/clarreo_config.py Added CLARREO mission-specific CorrectionConfig builder + optional JSON export.
tests/test_correction/clarreo/_pipeline_helpers.py New upstream/downstream pipeline runner helpers for integration tests.
tests/test_correction/clarreo/_image_match_helpers.py New helpers for case discovery, error injection/variation, and image-match execution.
tests/test_correction/clarreo/init.py Package marker for CLARREO-specific tests.
tests/test_correction/_synthetic_helpers.py New synthetic data and synthetic image-matching generators for upstream tests.
tests/integration/init.py Package marker for integration tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 85.09969% with 142 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.52%. Comparing base (d2e964c) to head (47c60a3).
⚠️ Report is 1 commits behind head on 122-refactor-correction-package-epic.

Files with missing lines Patch % Lines
tests/test_correction/clarreo/_pipeline_helpers.py 65.85% 39 Missing and 3 partials ⚠️
tests/test_correction/_synthetic_helpers.py 61.25% 30 Missing and 1 partial ⚠️
tests/test_correction/test_pipeline.py 72.22% 25 Missing ⚠️
...ts/test_correction/clarreo/_image_match_helpers.py 84.40% 10 Missing and 7 partials ⚠️
tests/test_correction/test_kernel_ops.py 88.88% 9 Missing ⚠️
...sts/test_correction/clarreo/test_clarreo_dataio.py 63.15% 7 Missing ⚠️
tests/test_correction/clarreo/test_clarreo_e2e.py 91.78% 6 Missing ⚠️
tests/test_correction/conftest.py 76.92% 3 Missing ⚠️
curryer/correction/config.py 66.66% 0 Missing and 1 partial ⚠️
tests/test_correction/test_pairing.py 97.91% 0 Missing and 1 partial ⚠️

❌ Your project status has failed because the head coverage (76.52%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                           Coverage Diff                            @@
##           122-refactor-correction-package-epic     #146      +/-   ##
========================================================================
- Coverage                                 76.89%   76.52%   -0.37%     
========================================================================
  Files                                        81       90       +9     
  Lines                                     12828    12303     -525     
  Branches                                   1354     1331      -23     
========================================================================
- Hits                                       9864     9415     -449     
+ Misses                                     2462     2388      -74     
+ Partials                                    502      500       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mmaclay and others added 3 commits April 3, 2026 16:17
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>
@mmaclay
Copy link
Copy Markdown
Collaborator Author

mmaclay commented Apr 3, 2026

@copilot apply changes based on the remaining unresolved comments in this thread

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 3, 2026

@copilot apply changes based on the remaining unresolved comments in this thread

Applied all remaining unresolved changes in commit 94cb36b:

  1. root_dir fixture consolidation — Moved the root_dir fixture into conftest.py as a session-scoped fixture (with a clarifying docstring confirming parents[2] is the repo root), and removed the duplicated definitions from test_results_io.py, test_pipeline.py, and test_kernel_ops.py.

  2. clarreo_cfg mutation fixes in test_results_io.py — Both test_checkpoint_save_load_cleanup and test_checkpoint_load_missing_returns_none now call clarreo_cfg.model_copy(deep=True) before mutating the config, preventing state leakage between tests.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 19 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +17 to +46
def test_generate_clarreo_config_json(tmp_path, clarreo_gcs_data_dir, clarreo_generic_dir):
"""Generate the CLARREO config JSON and validate structure end-to-end."""
output_path = tmp_path / "configs" / "clarreo_correction_config.json"

config = create_clarreo_correction_config(clarreo_gcs_data_dir, clarreo_generic_dir, config_output_path=output_path)

assert output_path.exists()

with open(output_path) as fh:
config_data = json.load(fh)

assert "mission_config" in config_data
assert "correction" in config_data
assert "geolocation" in config_data
assert config_data["mission_config"]["mission_name"] == "CLARREO_Pathfinder"

corr = config_data["correction"]
assert isinstance(corr.get("parameters"), list)
assert len(corr["parameters"]) > 0
assert corr["earth_radius_m"] == 6378140.0
assert corr["performance_threshold_m"] == 250.0
assert corr["performance_spec_percent"] == 39.0
assert config_data["geolocation"]["instrument_name"] == "CPRS_HYSICS"

reloaded = correction.load_config_from_json(output_path)
assert reloaded.n_iterations == config.n_iterations
assert len(reloaded.parameters) == len(config.parameters)
assert reloaded.earth_radius_m == 6378140.0
reloaded.validate()

Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_generate_clarreo_config_json currently asserts basic round-trip properties but wouldn’t catch schema drift that makes the reloaded config unusable (e.g., OFFSET_KERNEL missing data.field, or missing kernel mappings leading to config_file=None). Consider asserting that each reloaded parameter has the expected ptype, config_file (for kernel-based params), and required data.field for OFFSET_KERNEL/OFFSET_TIME, so this test will fail if the JSON can’t be used to run correction.loop().

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit b7d59f5. The new assertions loop over every reloaded parameter and check:

  • ptype is a valid ParameterType enum value
  • config_file is not None for CONSTANT_KERNEL and OFFSET_KERNEL params
  • data.field is not None for OFFSET_KERNEL and OFFSET_TIME params
  • At least one parameter of each expected type is present

While writing these assertions I found and fixed three bugs in load_config_from_json that the new checks revealed:

  1. OFFSET_KERNEL config_file: the kernel-mapping lookup was failing because the param name (filename) didn't match the mapping keys (azimuth/elevation). Added a fallback to read config_file directly from the JSON.
  2. data.field: was being read via application_target.field_name (a different schema), but the serialized JSON uses a top-level "field" key. Fixed to check "field" first.
  3. current_value: was being read via "initial_value" but the JSON uses "current_value". Fixed with a "current_value" primary lookup and "initial_value" fallback.

…bugs in load_config_from_json

Agent-Logs-Url: https://github.com/lasp/curryer/sessions/bbf5eb75-f1b0-46f8-af24-f2758fd30858

Co-authored-by: mmaclay <21048535+mmaclay@users.noreply.github.com>
…nability; streamline field retrieval in config.py
@mmaclay mmaclay marked this pull request as ready for review April 3, 2026 23:29
@mmaclay mmaclay merged commit 0a09d1c into 122-refactor-correction-package-epic Apr 3, 2026
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants