Warn about weak alignments and reject ambiguous ones - #349
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
minhuanli
left a comment
There was a problem hiding this comment.
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=Noneis 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
tto 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 tophases", which isn't enough to act on. A one-line worked example would help a lot. - No
DataSetinterface. Every other member ofrs.algorithmstakes aDataSet. 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. ADataSetentry point (orrs.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.
c88ee59 to
0dcd4b1
Compare
! This PR was vibe-coded.
Stack
Part 11 of 11 for #31. Depends on #360 and is the final integration layer.
Merge bottom-up.
What this implements
This final layer adds the reliability contract around the complete alignment pipeline:
Closes #31. Related: #174.
Reviewer focus
Default gates
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
Automated tests