Skip to content

PR-19: migrate ephemeris core to v2 (#454)#519

Open
charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr19-migrate-frames-core-v2from
charlie/issue-454-v2-pr20-migrate-ephemeris-core-v2
Open

PR-19: migrate ephemeris core to v2 (#454)#519
charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr19-migrate-frames-core-v2from
charlie/issue-454-v2-pr20-migrate-ephemeris-core-v2

Conversation

@charliecreates
Copy link
Contributor

Stack step PR-19 in the issue #454 migration chain.

Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

The migration to schemaVersion: 2 is consistent and preserves existing cases, but the v2 specs currently declare an effectively unvalidated contract.result (properties: {}), which can hide regressions in returned structures. The hard-coded 1e-12 compare tolerances across all ephemeris methods may be too strict and could lead to flaky parity checks depending on numeric stability. There is also significant duplication of identical setup/defaults/workflow blocks across files, increasing the risk of future configuration drift.

Additional notes (1)
  • Maintainability | packages/parity-checking/specs/methods/ephemeris/spkgeo@v2.yml:12-23
    The v1 specs referenced a shared workflow via uses: - workflows/legacy/ephemeris.spk-read.basic@v1, while v2 inlines setup.kernels and a single invokeLegacyCall step. This is fine, but it increases duplication across nine files; any future change to kernels/workflow will require editing all specs and risks drift (one file missing a kernel, different tolerances, etc.).
Summary of changes

What changed

  • Migrated parity-checking method specs for ephemeris SPK read calls from v1 to v2 schema:
    • Replaced spkez, spkezp, spkezr, spkgeo, spkgps, spkpds, spkpos, spksfs, spkssb @v1.yml specs with corresponding @v2.yml files.
  • New v2 specs consistently include:
    • schemaVersion: 2, manifest, and contract sections.
    • Explicit setup.kernels pointing at $FIXTURES/basic-time and $FIXTURES/ephemeris-de440s.
    • Default comparison tolerances (defaults.compare.tolAbs/tolRel set to 1e-12).
    • A workflow using a single step: - op: invokeLegacyCall.
  • Test cases and args are preserved from v1 to v2 for each method (same IDs and argument lists), with expect: { ok: true } retained.

Comment on lines +5 to +12
contract:
contractMethod: ephemeris.spkez
canonicalMethod: ephemeris.spkez
aliases: []
result:
type: object
properties: {}
setup:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

contract.result is declared as an empty object schema (type: object + properties: {}) for a method that (by contract name) returns structured ephemeris outputs. If the v2 runner uses contract.result for validation and/or comparison shaping, this effectively disables result-shape validation and can allow regressions to slip through unnoticed (e.g., missing fields, wrong nesting, wrong array lengths).

Suggestion

Define a minimal but meaningful contract.result schema for spkez (and similarly for the other methods) that at least asserts the presence of expected top-level fields and basic types/array lengths (even if you keep it permissive beyond that). For example:

  • Add required properties like state (array length 6), lt (number), etc., according to what ephemeris.spkez returns in your contract.

If you want, reply with "@CharlieHelps yes please" and I can add a commit that updates all these v2 specs with appropriately minimal result schemas (consistent across the ephemeris methods).

Comment on lines +16 to +20
defaults:
compare:
tolAbs: 1e-12
tolRel: 1e-12
workflow:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these specs hard-code defaults.compare.tolAbs/tolRel to 1e-12. For ephemeris outputs, relative tolerances at this level can be unrealistically strict depending on units and magnitudes, and may produce flaky parity checks across platforms/BLAS/libm differences or kernel updates. If these are intended as “universal defaults”, consider centralizing or documenting why 1e-12 is correct for every method here.

Suggestion

Consider either:

  1. moving these tolerances to a shared default (so you can tune once), or
  2. setting per-method tolerances that match expected numeric stability (e.g., looser for state vectors, possibly tighter for light-time scalars).

If you want, reply with "@CharlieHelps yes please" and I can add a commit that extracts these defaults (if your spec system supports inheritance/includes) or adjusts tolerances per method with a short rationale comment in each spec.

Comment on lines +16 to +19
defaults:
compare:
tolAbs: 1e-12
tolRel: 1e-12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All migrated specs hardcode the same defaults.compare tolerances (1e-12). That may be unrealistically strict for some SPK computations depending on platform differences, kernel versions, or reference frame handling, and could produce flaky parity tests.

If your v2 runner uses these tolerances for numeric deep-compare, consider either (a) centralizing tolerances, or (b) tuning per-method/per-field where needed.

Suggestion

Consider moving tolerances to a shared default (if supported by the framework) or loosening them to a known-stable value for SPK outputs (commonly 1e-9 to 1e-12 depending on units/scale), and override only where necessary. If field-level tolerances are supported, apply tighter tolerances to angles/normalized values and looser tolerances to position components.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit that centralizes these tolerances (or adjusts them consistently across these specs).

@github-actions
Copy link

orrery preview: https://pr-519.orrery-c4f.pages.dev/

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.

1 participant