fix(#304): implement ReferenceConstraint residuals and verify post-solve#305
Merged
Conversation
ReferenceConstraint.evaluate()/.is_satisfied() were stubs raising NotImplementedError, with comments pointing at the long-closed "Issue J" (#157). The forward solvers for every reference-constraint mode (incidence/emergence/specular/psi/omega) already existed; the dispatcher routed those modes to dedicated _solve_* functions and never called the stubs. - mode.py: evaluate() now returns real pseudo-angle residuals via reference.py (incidence/emergence/specular/psi/naz/omega); is_satisfied() delegates to it; remove stale "Issue J" comments. - forward.py: wire reference constraints into the post-solve verification loop (skip "psi", enforced upstream as a validation filter and subject to +/-360 deg azimuthal wraparound; guard unset reference vectors); fix the stale "not yet implemented" comment. - tests: replace the two NotImplementedError stub tests with real residual tests; fix is_implemented parametrization (psi True with azimuth, naz False); add cross-module regression test_regression_issue_304. naz still has no forward solver (is_implemented stays False); no demo geometry uses a naz mode, so nothing is broken. Contributed by: OpenCode (argo/claudeopus48)
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.
Reframing the issue
Issue #304 reported that
ReferenceConstraint.evaluate()/.is_satisfied()were stubs raising
NotImplementedError, concluding the reference-constraintsolvers were never finished. Investigation showed the premise is inverted:
(
incidence,emergence,specular,psi,omega) already exist inforward.py(_solve_surface,_solve_psi_mode,_solve_omega_mode,_solve_free_detectors,_solve_qaz_mode). The dispatcher routes thosemodes to the dedicated solvers and never calls the stubbed methods.
ConstraintSet.is_implemented()/ReferenceConstraint.is_implemented()already return
Trueonce the reference vector (azimuth/surface_normal)is set.
psic,fourcv,sixc,zaxis,s2d2,kappa*)solves end-to-end through
forward()once its reference vector is set —verified across all of them.
The real defect was therefore dead code + stale references, not a missing
solver. The two stub methods were reachable only from two tests that asserted
they raise, and the comments pointed at the long-closed "Issue J" (#157).
What this PR does
mode.py:evaluate()returns real pseudo-angle residuals viareference.py(
incidence/emergence/specular/psi/naz/omega);is_satisfied()delegates to it; the stale "Issue J" comments are removed.
forward.py: reference constraints are wired into the post-solve verificationloop (
_validate_solutions), so reference modes get the same residual sanitycheck as bisect/sample constraints.
psiis intentionally skipped there— it is enforced upstream as a validation filter in
_solve_psi_mode, and itsmotor-frame residual is subject to ±360° azimuthal wraparound, so a naive
per-solution check is unreliable. Unset reference vectors are guarded.
NotImplementedErrorstub tests are replaced with real residualtests; the
is_implementedparametrization is corrected (psiisTruewhenazimuthis set;nazisFalse). A new cross-module regression filetests/test_regression_issue_304.pyconfirms every affected mode solveswithout spurious
ConstraintViolationand that the loop rejects a badreference solution.
Known remaining gap (out of scope, not a regression)
nazhas no forward solver —forward.pyhas no_solve_naz_mode, andReferenceConstraint.is_implemented()deliberately returnsFalsefor it.evaluate()will compute anazresidual (used for verification only), but nodemo geometry's YAML declares a
nazmode, so nothing is broken. Per thepre-1.0 no-volunteered-follow-up policy, this is noted here rather than filed as
a new issue.
QC
pytest -m slow_benchmark: 3 passed (hot path:forward.py/mode.py).pre-commit run: all hooks pass.Contributed by: OpenCode (argo/claudeopus48)