Skip to content

Warn about weak alignments and reject ambiguous ones - #349

Open
tjlane wants to merge 1 commit into
rs-station:codex/phase-alignment-10-alignment-optionsfrom
tjlane:codex/phase-alignment
Open

Warn about weak alignments and reject ambiguous ones#349
tjlane wants to merge 1 commit into
rs-station:codex/phase-alignment-10-alignment-optionsfrom
tjlane:codex/phase-alignment

Conversation

@tjlane

@tjlane tjlane commented Aug 15, 2026

Copy link
Copy Markdown
Member

! This PR was vibe-coded.

Stack

Part 11 of 11 for #31. Depends on #360 and is the final integration layer.

  1. Check when alternative indexing or origin shifts are possible #351 ambiguity predicates
  2. Match and normalize reflections for reindexing comparisons #352 reindexing score primitives
  3. Choose and apply the best reindexing by correlation #353 public reindexing API
  4. Reject unsuitable data and unclear reindexing results #354 reindexing hardening
  5. Calculate phase agreement over symmetry-allowed origin shifts #355 constrained origin Fourier grid
  6. Search large origin-shift grids with limited memory #356 bounded-memory FFT
  7. Refine and rank possible origin shifts #357 origin refinement and ranking
  8. Match phase data and apply a chosen origin shift #358 DataSet phase matching and origin application
  9. Automatically reindex and align phases to a common origin #359 public phase-alignment orchestration
  10. Support phase weights and optional hand inversion #360 weighting and hand options
  11. this PR: confidence gates and final contract

Merge bottom-up.

What this implements

This final layer adds the reliability contract around the complete alignment pipeline:

  • validates the configurable number of refinement starts and requires at least two, so a competing solution cannot be hidden by a one-candidate cap;
  • adds phase and reindex warning, rejection, and best-versus-runner-up gap thresholds;
  • emits LowCorrelationWarning for usable but weak solutions and raises NoClearSolutionError when no unique solution is supported;
  • preserves caller-facing warning attribution while reusing the shared reindex scorer; and
  • finalizes the public result diagnostics and documentation, including the Phenix sign convention and bounded-search limitation.

Closes #31. Related: #174.

Reviewer focus

  • Whether best correlation and best-minus-runner-up gap are the right independent acceptance gates.
  • The requirement of at least two retained refinement starts when a uniqueness threshold is enabled.
  • Warning/error thresholds, caller-facing stack levels, and the statement that bounded multistart refinement is not a formal global certificate.

Default gates

stage warn below CC reject below CC reject below gap
reindexing 0.50 0.20 0.05
origin shift 0.50 0.20 0.15

These are conservative, user-overridable empirical defaults from seeded 6OVT noise experiments, not universal confidence estimates. Across 1,400 phase trials at 60 to 120 degrees noise, all 475 accepted solutions were correct and all 467 incorrect selections were rejected. Across 500 high-noise reindexing trials, none of 59 incorrect selections passed both gates.

Stack-wide validation

  • Origin classification agreed with CCTBX for all 230 reference space groups and all 3,179,520 denominator-24 grid comparisons, with zero mismatches.
  • Exact synthetic phase recovery passed 460 cases across all 230 groups; maximum residual was 3.278e-7 degrees.
  • The constrained per-coset P 21 search matched a 65,536-point dense reference in all 160 noisy trials; the historical shared seed missed 34.
  • A Phenix 2.0-5936 sign check recovered the injected P 61 origin shift (0, 0, -0.137), with MLAD 0 and coordinate RMSD 0.000002 A.
  • On 6OVT, reciprocalspaceship and CCP4 Pointless selected the same indexing coset. The full reindex-then-origin pipeline recovered all amplitudes exactly and phases to 4.657e-9 degrees in memory.

Automated tests

  • Focused stacked alignment suite: 950 passed.
  • All algorithm tests: 996 passed.
  • Full repository suite: 23,374 passed, 51 skipped, 53 xfailed. Six existing joblib cases were blocked only by sandbox semaphore permissions; all 11 tests in the implicated I/O modules passed with normal system access.
  • The combined production code and documentation are byte-for-byte identical to the previously green PR head; tests were reorganized by review layer and expanded by five cases.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.09677% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (codex/phase-alignment-10-alignment-options@b0a1ae4). Learn more about missing BASE report.

Files with missing lines Patch % Lines
reciprocalspaceship/algorithms/phase_alignment.py 87.09% 4 Missing ⚠️
Additional details and impacted files
@@                              Coverage Diff                              @@
##             codex/phase-alignment-10-alignment-options     #349   +/-   ##
=============================================================================
  Coverage                                              ?   89.06%           
=============================================================================
  Files                                                 ?       43           
  Lines                                                 ?     3530           
  Branches                                              ?        0           
=============================================================================
  Hits                                                  ?     3144           
  Misses                                                ?      386           
  Partials                                              ?        0           
Flag Coverage Δ
unittests 89.06% <87.09%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tjlane tjlane self-assigned this Aug 15, 2026
@tjlane
tjlane requested a review from minhuanli August 15, 2026 02:31

@minhuanli minhuanli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @tjlane — I checked this out and ran it against the repo's test data plus some independent probes. The core of the PR is solid: the allowed-origin construction is correct and complete, which is the hard, easy-to-get-wrong part. I verified the coset enumeration independently against International Tables' permissible origins and it matches everywhere I looked (P-1: 8; P2₁: 4 × free b; P4₃2₁2: 4; P2₁2₁2₁: 8; P3/R3:H: 3 × free c; P6₁/P6₃: 1 × free c; P3₂21/P6₃22: 2; P4₃32/Ia-3d/P2₁3: 2). It also handles the non-axis-aligned cases correctly — R 3:R gives the polar direction (1,1,1)/√3, and the periodic-image logic in _candidate_translations is there for exactly that reason.

I also checked that the search is complete over the manifold, not just over the coset list: against a brute-force scan of every coset × 1/360 polar grid, align_phases hit the global optimum in 0/30 trials suboptimal for P2₁, P3, P6₁, R3:R and C2 at up to 60° phase noise. And end-to-end on a real map (3KXE, P2₁2₁2₁, map → SFs → align → map) the CC against the reference went from −0.05 to 1.000.

So the origin part works. My main concern is what happens around it.

1. Handedness is not searched (please highlight this)

The Euclidean normalizer of most chiral space groups contains inversion; this PR implements only its translation subgroup. That means the inverted-hand solution — an equally valid solution of the same SAD experiment, and the single most common reason two isomorphous phase sets don't overlay — is invisible to align_phases.

Feeding it −φ_ref vs φ_ref in P2₁2₁2₁:

align_phases(H, -P_ref, P_ref, sg, weights=F_ref)
  -> t = (0.0, 0.5, 0.0),  weighted MPD after = 67.8°

No exception, no warning, no score — just a plausible-looking translation. Since #31's motivating case is SAD phasing of isomorphous structures, where the hand ambiguity is routine, I think shipping this as the answer to #31 will surprise people.

This is cheap to add: run the whole search a second time on −φ_moving and keep whichever scores better, behind something like search_inversion=True. At minimum it needs a Limitations section in the docstring.

2. Alternate indexing / the rotational part of the normalizer is not searched (please highlight this)

Same root cause. Where the lattice holohedry exceeds the Laue group (P3, P4, P6, R3, P2₁2₁2 with a≈b, …), the normalizer contains genuine rotations, and the corresponding reindexing operators produce alternative valid solutions that are not origin shifts.

In P6₁ (Laue 6/m, holohedry 6/mmm), reindexing by (h,k,l) → (k,h,−l) on 6OVT:

-> t = (0.0, 0.0, 0.9999),  weighted MPD after = 54.6°

Again silent. I don't think this PR has to solve reindexing — it's arguably a separate function — but the docstring should say plainly that only the translation part of the normalizer is searched, so users in the merohedral groups know they may need to reindex first.

3. No score is returned — this is what makes 1 and 2 dangerous

Both failures above are indistinguishable from a perfect hit at the API boundary. I also accidentally asked for a disallowed shift in P4₃2₁2 and got back (½,½,½) at an 82.5° residual, quite happily.

Please return the objective value, and ideally the ranked candidate list with scores — the gap between best and runner-up is the thing that tells you whether the answer is trustworthy. Right now every caller has to recompute the residual by hand to find out whether the result means anything.

4. Polar branch degrades earlier than an exhaustive search

Because a single 3D FFT peak seeds every coset, the polar refinement is a local search from one shared starting point. P2₁, 300 reflections, recovery of the true shift:

phase noise align_phases exhaustive
70° 38/40 40/40
80° 38/40 38/40
90° 26/40 33/40
100° 10/40 22/40

Equivalent below ~80° mean phase error, so this is fine for good phases — but 80–90° is exactly where a mediocre model or an experimental SAD map lives.

Suggestion that fixes this and item 5: instead of one 3D FFT over the whole cell, phase-modulate by each coset and run a 1D/2D FFT restricted to the polar subspace. That's a genuine global search over the constrained manifold, and it's orders of magnitude smaller than the current grid.

5. FFT grid cost

next_fast_len(2·max|h|+1) is taken per axis from the max index on that axis, so a single stray high-index reflection inflates the whole grid. In P1 at max|h| = 160 I measured 1.67 GB peak RSS / 2.4 s. The polar-subspace FFT above would make this a non-issue.

6. Smaller API points

  • weights=None is a questionable default. Unit weights let thousands of weak high-resolution terms drown out the strong low-resolution ones that actually determine the origin. The conventional choice is |F_ref|·|F_moving| (or E-values / FOM). At least document it.
  • Sign convention isn't documented. I had to determine it empirically: if the moving map is ρ_ref(x − s), the function returns t = −s (mod 1), and the aligned map is the moving map translated by +t — i.e. add t to the fractional coordinates of the moving model to superimpose it on the reference. The PR description notes the sign differs from Phenix's convention, but the docstring says only "translation added to phases", which isn't enough to act on. A one-line worked example would help a lot.
  • No DataSet interface. Every other member of rs.algorithms takes a DataSet. This takes bare arrays and requires the caller to do the inner join on Miller index themselves — silently wrong if the two sets aren't in the same ASU or the same order. A DataSet entry point (or rs.DataSet.align_phases(other, ...)) would fit the library better.

To summarize: the crystallography of the origin manifold is right and I'd be happy to see it merged on that basis. Before merge I'd like (1) documented limitations on hand and reindexing, ideally with an opt-in inversion search, and (2) a returned score. The polar-subspace FFT is the change I'd most like to see beyond that, since it addresses items 4 and 5 together.

@tjlane
tjlane force-pushed the codex/phase-alignment branch from c88ee59 to 0dcd4b1 Compare August 22, 2026 22:49
@tjlane tjlane changed the title Add constrained phase-origin alignment Add phase-alignment confidence gates Aug 22, 2026
@tjlane
tjlane changed the base branch from main to codex/phase-alignment-10-alignment-options August 22, 2026 23:07
@tjlane tjlane changed the title Add phase-alignment confidence gates Warn about weak alignments and reject ambiguous ones Aug 24, 2026
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.

Align Phases of Isomorphous Structures

2 participants