fix(#284) split kappa-arm tilt direction from equivalent-Eulerian chi axis#286
Merged
Conversation
… axis Issue #284 reported that kappa4cv bisecting and kappa6c bisecting_vertical lost solutions for sapphire asymmetric reflections (0,1,2), (0,0,6), (1,1,3) in v0.11.0. Root cause: issue #252 conflated two distinct concepts in the single YAML field 'kappa_chi_eq': 1. The kappa-arm tilt direction used by Walko's formula 'n_kappa = cos(α)*n_komega_unsigned + sin(α)*tilt_direction'. For kappa4cv this is +vertical (kappa arm lies in the transverse- vertical plane). 2. The equivalent-Eulerian chi pseudo-angle axis the kappa-to-Eulerian decomposition rotates about for the virtual chi angle. For kappa4cv this must be +longitudinal to match fourcv's chi axis (the non-kappa Eulerian preset kappa4cv is mechanically equivalent to). Setting both to the same value (+vertical) broke the second role: the kappa equivalent-Eulerian decomposition's reachable Bragg locus shrank away from the locus reachable by fourcv bisecting, making asymmetric reflections decline. Implementation (Option C from the investigation): - src/ad_hoc_diffractometer/geometry_loader.py: * Add an optional top-level YAML field 'kappa_eulerian_chi' that sets the convention's n_chi_eq verbatim when present. * When absent, derive n_chi_eq automatically as the first basis direction perpendicular to n_komega in the conventional order (+longitudinal, +vertical, +transverse) -- yields +longitudinal for every shipped kappa preset. * Helper _derive_kappa_eulerian_chi() implements the derivation. - src/ad_hoc_diffractometer/geometries/schema.json: document the new field and clarify kappa_chi_eq's role. - src/ad_hoc_diffractometer/geometries/{kappa4cv,kappa6c,kappa4ch}.yml: reword the kappa_chi_eq header comment to name only the kappa-arm tilt role; cite the new field for the eulerian-equivalent role. - src/ad_hoc_diffractometer/geometries/__init__.py: rewrite the 'Kappa angle (alpha) convention' section to distinguish the two axes; add a 'Equivalent-Eulerian chi axis vs kappa-arm tilt direction (issue #284)' subsection. - src/ad_hoc_diffractometer/kappa.py: update KappaPseudoAngleConvention docstring -- the rotation-matrix identity is solvable for any n_chi_eq perpendicular to n_komega, not only the in-plane choice. - src/ad_hoc_diffractometer/forward.py: pragma the unreachable degenerate branch of _solve_bisecting_analytic (w_perp_norm < 1e-12 or q_perp_norm < 1e-12) -- no shipped geometry, mode, or reflection lands there after the n_chi_eq correction. Tests (per-test audit; no algorithmic test edits): - tests/test_regression_issue_284.py (new): exact issue reproducer, cross-equivalence kappa<->fourcv/fourch/psic, kappa-arm-axis invariance, Q-equivalence sweep restored from the pre-#252 test_regression_issue_241.py file, YAML auto-derivation / explicit-override / numeric-vector / invalid-form / fallback / no-perpendicular-basis cases. - tests/test_geometry_loader.py::test_kappa_chi_eq_numeric_vector_form: rewritten to assert the kappa stage axis (the field's actual role) and the auto-derived n_chi_eq. - tests/test_regression_issue_267.py: hand-built kappa4cv and kappa6c reference geometries updated to use n_chi_eq = +LONGITUDINAL (matching the new auto-derivation; kappa4ch already had the right value because tilt direction and eulerian chi coincide there). - tests/test_regression_issue_252.py::test_eulerian_kappa_round_trip: asserts the rotation-matrix identity rather than the angle identity (each branch is a valid decomposition; +1 branch can land on the chi-mirrored angles for some kappa presets). - tests/test_factories.py::test_make_geometry_kappa_alpha_forwarded: docstring rewritten to distinguish the two fields. Other: - .gitignore: broaden '.coverage' to '.coverage*' (covers context- enabled coverage runs and per-process suffixes). - CHANGES.md: replace the bullet-style 'Unreleased' header with an HTML comment block (matches the user's CHANGES.md style preference). Verification (local): - 2571 passed, 2 skipped, 3 deselected; 100% line + branch coverage. - slow_benchmark 3/3 passed. - pre-commit clean. - US-English grep clean over every changed file. - Sphinx 'make clean html' build succeeded; zero warnings; zero RST role leaks (both grep patterns from AGENTS.md). Contributed by: OpenCode (argo/claudeopus47)
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
Issue #284 reported that
kappa4cv bisectingandkappa6c bisecting_verticallost solutions for sapphire asymmetric reflections —(0, 1, 2),(0, 0, 6),(1, 1, 3)all return zero solutions in v0.11.0.Root cause
Issue #252 conflated two distinct concepts in the single YAML field
kappa_chi_eq:n_kappa = cos(α)*n_komega_unsigned + sin(α)*tilt_direction. Forkappa4cvthis is+vertical(kappa arm lies in the transverse-vertical plane, per Walko 2016 Fig. 3).kappa4cvthis must be+longitudinalto matchfourcv's chi axis (the non-kappa Eulerian presetkappa4cvis mechanically equivalent to).Setting both to the same value (
+vertical) broke the second role: the kappa equivalent-Eulerian decomposition's reachable Bragg locus shrank away from the locus reachable byfourcv bisecting. Cross-checked against libhklK4CV, which has chi-eq =+longitudinaland finds the missing solutions.For
kappa4chthe two roles coincide (both+longitudinal), so it was already correct.Fix (Option C from investigation)
Decouple the two meanings without a breaking schema change:
kappa_eulerian_chithat setsn_chi_eqverbatim when present.n_chi_eqas the first basis direction perpendicular ton_komegain the conventional order(+longitudinal, +vertical, +transverse)— yields+longitudinalfor every shipped kappa preset.kappa_chi_eqfield continues to drive Walko's kappa-arm tilt formula in_resolve_axis(unchanged).Source changes
geometry_loader.py: parse the new field, derive when absent, buildKappaPseudoAngleConventionwith the resolvedn_chi_eq.geometries/schema.json: document the new field; clarifykappa_chi_eq's role.geometries/{kappa4cv,kappa6c,kappa4ch}.yml: reword thekappa_chi_eqheader comment to name only the kappa-arm tilt role.geometries/__init__.py: distinguish the two axes in the package docstring.kappa.py:KappaPseudoAngleConventiondocstring — the rotation-matrix identity is solvable for anyn_chi_eqperpendicular ton_komega, not only the in-plane choice.forward.py:# pragma: no coverthe unreachable degenerate branch of_solve_bisecting_analytic(no shipped geometry / mode / reflection lands there post-fix).Test changes
tests/test_regression_issue_284.py(67 cases):kappa4cv ↔ fourcv,kappa6c ↔ psic,kappa4ch ↔ fourch.test_regression_issue_241.pyfile.+longitudinalis parallel ton_komega, andGeometrySchemaErrorwhen no basis direction is perpendicular.test_geometry_loader.py::test_kappa_chi_eq_numeric_vector_form— rewritten to assert the field's actual current role (kappa stage axis construction) plus the auto-derivedn_chi_eq.test_regression_issue_267.py— hand-built_reference_kappa4cv/_reference_kappa6cupdated ton_chi_eq = +LONGITUDINAL(kappa4ch unchanged).test_regression_issue_252.py::test_eulerian_kappa_round_trip— asserts the rotation-matrix identity rather than the angle identity (both branches are valid decompositions; geometry-dependent which branch the+1parameter lands on).test_factories.py::test_make_geometry_kappa_alpha_forwarded— docstring rewritten to distinguish the two fields.Other
.gitignore— broaden.coverageto.coverage*(user edit, included here).CHANGES.md—Unreleasedsection restyled as an HTML comment block per the user's preference; one bullet inside for the Regression: kappa4cv/kappa6c bisecting modes lose solutions for sapphire reflections in v0.11.0 #284 fix.Local QC (all green)
pytest: 2571 passed, 2 skipped, 3 deselected; 100% line + branch coverage.pytest -m slow_benchmark: 3/3 passed.pre-commit run --all-files: clean.make -C docs clean html: succeeded, zero warnings, zero RST role leaks.Notes on related issues
(1, 1, 0)in horizontal-bisecting modes) does not reproduce against shipped v0.11.0; the downstreamKNOWN_FORWARD_GAPSentry appears to predate PR fix(#280) rotation composition order, ub_identity, and B-matrix orthogonalized frame #281's merge. Verified by running the reproducer against the PyPI 0.11.0 wheel: all three cases return 2 solutions.Contributed by: OpenCode (argo/claudeopus47)