Skip to content

Fix ORTEP arc boundary: use ellipsoid silhouette normal condition instead of Pz=0#17

Merged
dkratzert merged 1 commit into
mainfrom
copilot/fix-ellipsoid-intersection-lines
May 9, 2026
Merged

Fix ORTEP arc boundary: use ellipsoid silhouette normal condition instead of Pz=0#17
dkratzert merged 1 commit into
mainfrom
copilot/fix-ellipsoid-intersection-lines

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 9, 2026

Root cause

In _draw_principal_arcs the boundary between the solid (front) and absent (back) halves of each principal great-circle arc was determined by where the z-coordinate of the 3D point on the arc is zero (Pz = 0).

The correct ORTEP criterion is where the outward normal of the ellipsoid is perpendicular to the viewing direction, i.e. (U⁻¹P)_z = 0.

For a point P(t) = rᵢ·vᵢ·cos(t) + rⱼ·vⱼ·sin(t) lying on the ellipsoid surface:

(U⁻¹P)_z = (vᵢ[2] / rᵢ) · cos(t)  +  (vⱼ[2] / rⱼ) · sin(t)

Setting this to zero gives phi_n = atan2(vⱼ[2]·rᵢ, vᵢ[2]·rⱼ).

The old code computed phi_z = atan2(rⱼ·vⱼ[2], rᵢ·vᵢ[2]) — the roles of rᵢ and rⱼ are swapped compared to the correct formula.

Effect

Condition Arc endpoints
Old (Pz = 0) Fall inside the 2-D projected ellipse for anisotropic atoms
New (silhouette normal) Land exactly on the projected ellipse boundary ✓

The two conditions are identical for isotropic ellipsoids (rᵢ = rⱼ), which is why only some ellipsoids showed the wrong curves.

Fix

A 2-line change in _draw_principal_arcs:

# Before
Az = ri_3d * vi2
Bz = rj_3d * vj2

# After – swap ri_3d ↔ rj_3d so the atan2 encodes (U⁻¹P)_z = 0
Az_n = vi2 * rj_3d
Bz_n = vj2 * ri_3d

Tests

192 tests pass (1 pre-existing failure due to missing PIL dependency, unrelated to this change).

Copilot AI requested a review from dkratzert May 9, 2026 11:28
@dkratzert dkratzert marked this pull request as ready for review May 9, 2026 12:14
@dkratzert dkratzert merged commit 01492b0 into main May 9, 2026
9 checks passed
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.

2 participants