alignment: add plugin test suite with Halton sky sampling#2352
Merged
Conversation
Add JulianOffset to TransformTelescopeToCelestial in MathPlugin and all concrete implementations (Basic, Dummy, Nearest, SVD, BuiltIn) so tests can inject a fixed observation time instead of relying on the system clock. MathPluginManagement propagates the parameter through to the loaded plugin. Add SetDatabaseReferencePosition(IGeographicCoordinates) overload to InMemoryDatabase so callers can pass a structured position directly. Guard the plugin-hook extern "C" functions in SVD, Nearest, and BuiltIn with NO_PLUGIN_HOOKS so the same translation units can be compiled into test binaries without symbol collisions. Fix BuiltIn singular-matrix handling: the MatrixMatrixMultiply and inverse steps were gated on a non-null check that prevented them from running after the identity fallback on the non-singular path. Remove the outer else and let both steps always run. Add indicore to AlignmentDriver's public include path and install HaltonSequence.h alongside the other public alignment headers.
Fix two coordinate handling bugs in scopesim_helper revealed by the new test suite: AltAz: scopesim's azimuth convention places 0° at South (increasing clockwise West→North→East), while INDI's IHorizontalCoordinates places 0° at North. Apply +180° offset in apparentHaDecToMount so the simulated AltAz encoder matches what drivers expect. Also add an early return to prevent fallthrough into the equatorial code path. EQ_GEM: mountToApparentHaDec was assigning prio = primary only inside the flipped-pier else branch, leaving prio uninitialised on the normal (no-flip) path. Move the assignment before the flip check so it is always set.
81779fd to
5d9201f
Compare
Add a comprehensive test suite for the alignment math plugins (SVD, BuiltIn, Nearest) covering equatorial GEM, equatorial fork, AltAz, and meridian flip configurations across multiple geographic sites. Tests inject known mount errors (IH, ID, CH, NP, MA, ME) via scopesim_helper, align a subset of sky points, then validate round-trip accuracy on held-out points using a fixed RMS arcsecond target. Dual Halton sequences (bases 2/3 for alignment, 5/7 for validation) give uniform sky coverage without overlap. The QualityCheck test verifies Nearest meets a 100" RMS threshold. Add HaltonSequence.h (generic 2D quasi-random generator) and HOURS_TO_RAD / RAD_TO_HOURS macros to indicom.h. Wire up the test binary in CMakeLists.
After instrumentToObserved(), apparentHa holds corrected Azimuth in INDI convention (North=0). The subsequent rotateY() back to HA/Dec must use the corrected value, not the raw instrument axis prio. Convert from INDI North=0 to scopesim South=0 before forming the Vector. Without this fix, the pointing-model correction applied by instrumentToObserved was discarded before the AltAz→HA/Dec rotation, so AltAz mount errors were not reflected in the apparent HA/Dec output of the simulator.
…cToMount The AltAz path was computing the apparent Az/Alt from the HA/Dec vector rotation but returning it directly as the mount position, bypassing observedToInstrument(). This meant pointing-model error terms (IH, ID, CH, MA, ME) were never applied to AltAz encoder positions in the simulator. Apply observedToInstrument() on the apparent Az/Alt before writing primary and secondary, consistent with the equatorial path.
c20d777 to
77f1736
Compare
Contributor
|
Thank you for the changes. Are you using the Alignment module for any specific requirement when you ran into these issues? |
…site param - Remove unused INDI::IGeographicCoordinates site parameter from buildEntry; the generator already encodes site coordinates set in buildGenerator - Add SVD_AlignValidate_Arctic (lat 78.2) and SVD_AlignValidate_Antarctic (lat -77.8) to cover high-latitude mount geometry - Remove unused kLondon constant Fixes -Werror=unused-parameter in CI.
Contributor
Author
Ah sorry. The context is in a message on the indi dev forum. I am writing a new math plugin and when I wrote the tests for it they uncovered some issues in the existing plugins. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch establishes a test harness for INDI alignment math plugins and fixes two correctness bugs in the simulator and BuiltIn plugin that the tests exposed.
Interface changes
MathPlugin::TransformTelescopeToCelestialgains aJulianOffset = 0parameter, matching the existingTransformCelestialToTelescopesignature. This allows tests to inject a fixed Julian Date and get deterministic transforms independent of the system clock. All existing implementations updated; a TODO notes that a future refactor should move to an absolute Julian Date rather than an offset.InMemoryDatabase::SetDatabaseReferencePositiongains anIGeographicCoordinatesoverload alongside the existing(double lat, double lon)form.indicom.hgains angle-unit conversion macros:HOURS_TO_DEG,DEG_TO_HOURS,ARCMIN_TO_DEG,DEG_TO_ARCMIN,ARCSEC_TO_DEG,DEG_TO_ARCSEC,HOURS_TO_RAD,RAD_TO_HOURS(all guarded with#ifndef).BuiltInMathPlugingains aNO_PLUGIN_HOOKSguard around itsextern "C"factory functions, matching the pattern used by SVD and Nearest, so the translation unit can be linked directly into test binaries without symbol conflicts.Bug fixes
scopesim_helper.cpp— AltAz azimuth conventionapparentHaDecToMountwas producing azimuth in scopesim's internal convention (0° = South, clockwise) without converting to INDI'sIHorizontalCoordinatesconvention (0° = North). Added+Angle(180.0)to the primary output and areturnto prevent fallthrough into the equatorial code path.scopesim_helper.cpp— GEM meridian flipmountToApparentHaDecinitialisedprioinside anelsebranch, leaving it uninitialised when the flip condition was true. Movedprio = primarybefore the flip check so both branches are always covered.libs/alignment/BuiltInMathPlugin.cpp— singular matrix fallthroughCalculateTransformMatriceswrapped theMatrixMatrixMultiplyand inverse steps in anelseguarded by the singularity check, so a singular Alpha matrix silently skipped the Beta matrix calculation entirely. Removed theelse; the identity fallback is set and computation continues regardless.Test suite (
test/alignment/test_alignment_plugins.cpp)34 tests using a dual-Halton sequence design: alignment points drawn from
Halton(2,3), independent validation points fromHalton(5,7). This eliminates the correlation between training and test sets while giving good sky coverage with few points.Coverage includes:
Assertions use a fixed
targetRMSArcsecquality target rather than a fraction of injected error, with a P95 bound at1.5×the RMS target to catch outliers.Test plan
Expected: 34 tests pass. The two
DISABLED_Nearest_MeridianFliptests are intentionally skipped (NearestMathPlugin does not handle flip geometry reliably without dense coverage).