Skip to content

polar alignment plan#29

Open
peterlulham wants to merge 3 commits intomainfrom
5-polar
Open

polar alignment plan#29
peterlulham wants to merge 3 commits intomainfrom
5-polar

Conversation

@peterlulham
Copy link
Copy Markdown
Collaborator

@peterlulham peterlulham commented Mar 1, 2026

Plan: Polar Alignment Service Refactor & Implementation

Summary

Refactor the polar alignment feature from a stub flat file (astrolabe/services/polar.py) into a full sub-package (astrolabe/services/polar/) with a three-pose circle-fitting algorithm for robust axis error computation.

Key Changes

Architecture

  • Promote to sub-package: Move services/polar.pyservices/polar/ with four modules:
    • __init__.py — public API re-exports
    • types.pyPolarResult, _PoseObservation, _CircleFitResult dataclasses
    • math.py — pure geometric functions (pole fitting, alt/az error, confidence)
    • service.py — orchestration and backend integration
  • No breaking changes: Same public API, backward-compatible imports, optional parameters only.

Algorithm: Three-Pose Circle Fitting

  • Why three poses? Two poses have zero redundancy; mechanical error (backlash, flexure) corrupts the result with no way to detect it. Three poses give one degree of freedom, yielding a real fit residual and confidence estimate.
  • How it works: Fit a small circle through three field-centre positions on the celestial sphere. The circle's pole is the mount's actual rotation axis. Compare to celestial pole → alt/az corrections.
  • Implementation: Perpendicular bisecting planes + least-squares intersection. No SVD/eigenvalue solver; only stdlib math and 3D vector operations (cross-product, dot-product).

Robustness & Operational Considerations

  • Tracking state check: Asserts mount.tracking == True before any capture. Sidereal tracking is required to form a valid circle.
  • Mount settling time: Introduces settle_time_s (default 2.0 s) sleep after each slew, before capture. Mechanical vibrations damp, preventing star trails in the plate solve.
  • Latitude fallback: Service accepts explicit site_latitude_rad parameter (from CLI --lat or global config), falls back to mount state if available. Raises ServiceError if both are unavailable (not all mount drivers report latitude).

Testing

  • Unit tests (test_math.py): 13 cases for circle fitting, error projection, and confidence estimation.
  • Service tests (test_service.py): 14 cases including happy path, failure modes, tracking checks, latitude resolution, settle time verification.

Impact

  • New files: 8 (package + tests)
  • Deleted files: 1 (old stub)
  • Modified files: 0
  • Breaking changes: None
  • Dependencies: None added (stdlib only)

Next Steps

  1. Implement astrolabe/services/polar/ modules
  2. Create tests/services/polar/ test files
  3. Delete old astrolabe/services/polar.py
  4. Run full test suite to verify import resolution
  5. Verify CLI still works: astrolabe polar --ra-rotation-deg 15

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

Adds a detailed design/implementation plan document for refactoring the polar alignment service from a stub module into a astrolabe/services/polar/ sub-package and implementing a three-pose circle-fitting approach.

Changes:

  • Introduces docs/dev/plan_service_polar.md describing target package layout, APIs, algorithm approach, and proposed test coverage.
  • Documents service orchestration flow (tracking checks, settle time, latitude resolution) and suggested unit/service test cases.
Comments suppressed due to low confidence (6)

docs/dev/plan_service_polar.md:6

  • The PR description says this change implements the polar alignment service + tests, but this file is explicitly a “Layout & Architecture Specification” plan document. Either adjust the PR description to reflect that this PR is documentation-only, or update the doc header/status to match the intended scope of this PR.
**Date:** 2026-03-01  
**Status:** Layout & Architecture Specification  
**Scope:** Refactor `services/polar.py` → `services/polar/` package with internal structure  
**Breaking Changes:** None

docs/dev/plan_service_polar.md:27

  • The cross-doc references use “§…” (e.g., “docs/architecture.md §5.3”), but the referenced docs are formatted with numeric headings like “### 5.3 …” and don’t contain the “§” marker. Consider switching these references to the actual heading format (e.g., “docs/architecture.md 5.3”, “docs/conventions.md 2.1”) so they’re searchable and consistent.
The architecture doc (`§5.3`) describes a conceptual two-pose flow: capture/solve at pose A, rotate RA, capture/solve at pose B, compute error. This is geometrically sound in theory — if the commanded RA rotation is mechanically exact, two field-centre solves fully constrain the alt/az correction.

docs/dev/plan_service_polar.md:134

  • In the types.py spec snippet, _PoseObservation uses datetime.datetime for timestamp_utc, but the snippet doesn’t import datetime. Since this doc is meant as an implementation spec, include the needed import (and consider using datetime.datetime with tzinfo explicitly, as the tests do).
```python
@dataclass
class _PoseObservation:
    """Result of a single capture→solve at one RA position."""
    ra_rad: float
    dec_rad: float
    rms_arcsec: float
    timestamp_utc: datetime.datetime

docs/dev/plan_service_polar.md:183

  • There’s an internal inconsistency in the math spec: fit_polar_axis() says pose order “does not matter”, but _fit_circle_spherical() later defines the fit using “each consecutive pair of points (P_i, P_{i+1})”, which does depend on list order for N>3. Either (a) specify that points must be ordered (e.g., increasing RA along the rotation), or (b) change the fitting method description to be order-invariant (e.g., using all unique pairs or another formulation).
    Args:
        poses: Three or more _PoseObservation results at distinct RA
               positions (ICRS). Order does not matter.
        site_latitude_rad: Observer latitude (radians, positive north).

docs/dev/plan_service_polar.md:549

  • The example test helper _generate_circle_poses() can produce RA values outside the documented [0, 2π) range (it sets ra = pole_ra + dra and then converts to degrees without normalization). Since docs/conventions.md defines RA as [0, 2π), either normalize RA in the example (ra % (2π)) or note explicitly that the implementation must handle wrap-safe RA values.
            - math.sin(pole_dec) * math.sin(radius) * math.cos(angle),
        )
        ra = pole_ra + dra
        poses.append(_make_pose(math.degrees(ra), math.degrees(dec), minutes_offset=i * 5))

docs/dev/plan_service_polar.md:927

  • In the “Validation” section, the doc says astrolabe polar --ra-rotation-deg 15 should still return a not_implemented error “as before”, which contradicts the rest of this plan (and the PR description) that describes implementing the service + tests. Update these validation expectations to match the intended end state (implemented behavior) or clarify that this plan is staged across multiple PRs.
13. Verify CLI still works:
    - `astrolabe polar --ra-rotation-deg 15 --json` returns JSON envelope with `"not_implemented"` error (as before).
    - `astrolabe polar --ra-rotation-deg 15` returns stderr message (as before).


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

Copy link
Copy Markdown
Owner

@grantaj grantaj left a comment

Choose a reason for hiding this comment

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

Need to make sure that slewing for pose is not intercepted by Pointing service

We will need to handle cases where we get a bad frame - maybe that is something to add later

I assume this is just the service to find the polar pointing error? We need to convert that to azimuth elevation and have some feedback process for the mechanical movement by the user. This will require some kind of real time solving, on the back of some kind of streaming from the CCD. Again this is probably a separate issue.

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 10 out of 12 changed files in this pull request and generated 5 comments.


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

Comment thread astrolabe/cli/commands.py
Comment on lines 715 to 738
@@ -733,6 +738,20 @@ def run_polar(args) -> int:
return 0
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

run_polar() always treats the polar run as successful: the JSON envelope sets ok=True unconditionally and the function returns exit code 0 even when PolarAlignService.run() returns a failure PolarResult (e.g., alt_correction_arcsec is None). This makes CLI automation unable to detect plate-solve / circle-fit failures.

Consider deriving ok from the returned result (e.g., corrections/residual/confidence all non-None) and returning a non-zero exit code + ok=False/error payload when the service reports failure via PolarResult.message / None corrections.

Copilot uses AI. Check for mistakes.

def _rotate_ra(self, delta_rad: float, settle_time_s: float) -> None:
"""Slew the mount by delta_rad in RA, then wait for vibrations."""
state = self._mount.get_state()
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

_rotate_ra() assumes state.ra_rad and state.dec_rad are non-None, but MountState allows these to be None (e.g., when a backend can’t read coordinates). As written, target_ra = state.ra_rad + delta_rad will raise TypeError and slew_to(..., state.dec_rad) can pass None.

Please validate coordinates are available (and raise ServiceError with a clear message) before attempting the slew.

Suggested change
state = self._mount.get_state()
state = self._mount.get_state()
if state.ra_rad is None or state.dec_rad is None:
raise ServiceError(
"Mount coordinates are unavailable; cannot rotate in RA for polar alignment"
)

Copilot uses AI. Check for mistakes.
Comment on lines +81 to +91
result = self._solver.solve(request)

if not result.success:
return None

return _PoseObservation(
ra_rad=result.ra_rad,
dec_rad=result.dec_rad,
rms_arcsec=result.rms_arcsec or 0.0,
timestamp_utc=image.timestamp_utc,
)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

_capture_and_solve() returns a _PoseObservation even if a backend were to return SolveResult(success=True, ra_rad=None/dec_rad=None). Since SolveResult.ra_rad/dec_rad are typed as Optional[float], this can propagate None into the math layer and fail later in less obvious ways.

Consider treating missing ra_rad/dec_rad as a solve failure (return None or raise) and include result.message in the failure path so the user sees the solver’s reason.

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +90
return _PoseObservation(
ra_rad=result.ra_rad,
dec_rad=result.dec_rad,
rms_arcsec=result.rms_arcsec or 0.0,
timestamp_utc=image.timestamp_utc,
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

When SolveResult.rms_arcsec is None, _capture_and_solve() substitutes 0.0, which makes correction_confidence() treat the solve as perfect (solve_conf = 1.0) and can inflate confidence. Since the ASTAP backend can return success=True with rms_arcsec=None, this can happen in practice.

Consider using a conservative default penalty when RMS is unavailable, or exclude the RMS term from confidence computation when any pose lacks RMS data.

Copilot uses AI. Check for mistakes.
Comment on lines +122 to +150
# For 3 points the cross-product of two plane normals gives the
# direction of the pole. For more points use least-squares.
if len(points) == 3:
pole_cart = _fit_pole_cross(normals, offsets, vecs)
else:
pole_cart = _fit_pole_lstsq(normals, offsets, vecs)

pole_ra, pole_dec = _cart_to_radec(pole_cart)

# Radius = mean angular distance from each point to the pole.
ang_dists = [math.acos(max(-1.0, min(1.0, _dot(pole_cart, v)))) for v in vecs]
radius = sum(ang_dists) / len(ang_dists)

# A radius near π/2 means the points lie on a great circle, which
# is degenerate for small-circle fitting (the pole is ambiguous).
if abs(radius - math.pi / 2) < math.radians(1.0):
raise ValueError(
"Points lie on or near a great circle — cannot fit a small circle"
)

# Residual = RMS deviation from the mean radius.
residual = math.sqrt(sum((d - radius) ** 2 for d in ang_dists) / len(ang_dists))

return _CircleFitResult(
pole_ra_rad=pole_ra,
pole_dec_rad=pole_dec,
radius_rad=radius,
residual_rad=residual,
)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

With exactly 3 points, _fit_circle_spherical() computes the pole via _fit_pole_cross() using two bisector planes, which enforces equal dot-products to all three points; as a result ang_dists become equal and the computed residual_rad is (near-)zero for any non-degenerate 3-point input. This means the service’s residual_arcsec/fit-based confidence signal will almost always be ~0/1.0 and won’t detect an inconsistent third observation (backlash/flexure/bad solve), undermining the stated motivation for a 3-pose method.

To get a meaningful residual/confidence signal, consider either collecting N>=4 poses, or (even for N=3) estimating the pole via a least-squares objective that doesn’t force the circle to pass exactly through all points (e.g., solve using all bisector planes including wrap and compute residuals against that best-fit pole).

Copilot uses AI. Check for mistakes.
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