Conversation
There was a problem hiding this comment.
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.mddescribing 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.pyspec snippet,_PoseObservationusesdatetime.datetimefortimestamp_utc, but the snippet doesn’t importdatetime. Since this doc is meant as an implementation spec, include the needed import (and consider usingdatetime.datetimewithtzinfoexplicitly, 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 setsra = pole_ra + draand then converts to degrees without normalization). Sincedocs/conventions.mddefines 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 15should still return anot_implementederror “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.
grantaj
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -733,6 +738,20 @@ def run_polar(args) -> int: | |||
| return 0 | |||
There was a problem hiding this comment.
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.
|
|
||
| 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() |
There was a problem hiding this comment.
_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.
| 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" | |
| ) |
| 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, | ||
| ) |
There was a problem hiding this comment.
_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.
| 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, |
There was a problem hiding this comment.
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.
| # 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, | ||
| ) |
There was a problem hiding this comment.
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).
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
services/polar.py→services/polar/with four modules:__init__.py— public API re-exportstypes.py—PolarResult,_PoseObservation,_CircleFitResultdataclassesmath.py— pure geometric functions (pole fitting, alt/az error, confidence)service.py— orchestration and backend integrationAlgorithm: Three-Pose Circle Fitting
Robustness & Operational Considerations
mount.tracking == Truebefore any capture. Sidereal tracking is required to form a valid circle.settle_time_s(default 2.0 s) sleep after each slew, before capture. Mechanical vibrations damp, preventing star trails in the plate solve.site_latitude_radparameter (from CLI--lator global config), falls back to mount state if available. RaisesServiceErrorif both are unavailable (not all mount drivers report latitude).Testing
test_math.py): 13 cases for circle fitting, error projection, and confidence estimation.test_service.py): 14 cases including happy path, failure modes, tracking checks, latitude resolution, settle time verification.Impact
Next Steps
astrolabe/services/polar/modulestests/services/polar/test filesastrolabe/services/polar.pyastrolabe polar --ra-rotation-deg 15