Support nonstandard-orientation irrep matrices and symtexts by nlk36701 · Pull Request #74 · NASymmetry/MolSym

nlk36701 · 2026-08-18T20:05:48Z

Draft: looking for feedback on the overall approach before finalizing

Adds Symtext.nonstandard_symtext() : given a symtext in molsym's standard orientation, construct the corresponding symtext for the molecule's original (arbitrary) orientation.

The only function set I have tested this nonstandard symtext with so far are the cartesian_coordinates. In doing so, I added a more robust method for finding symmetry equivalent functions.

codecov · 2026-08-18T20:07:45Z

Codecov Report

❌ Patch coverage is 94.81481% with 14 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
molsym/salcs/nonstandard_frame.py	89.87%	8 Missing ⚠️
molsym/salcs/salc.py	85.71%	4 Missing ⚠️
molsym/salcs/spherical_harmonics.py	97.22%	1 Missing ⚠️
molsym/symtext/symtext.py	92.85%	1 Missing ⚠️

Flag	Coverage Δ
unittests	`87.15% <94.81%> (+0.66%)`	⬆️

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

Files with missing lines	Coverage Δ
molsym/salcs/cartesian_coordinates.py	`100.00% <100.00%> (ø)`
molsym/salcs/function_set.py	`95.89% <100.00%> (+11.67%)`	⬆️
molsym/salcs/polynomial_functions.py	`100.00% <100.00%> (+0.86%)`	⬆️
molsym/salcs/spherical_harmonics.py	`98.97% <97.22%> (+0.09%)`	⬆️
molsym/symtext/symtext.py	`79.79% <92.85%> (+1.53%)`	⬆️
molsym/salcs/salc.py	`92.16% <85.71%> (+1.11%)`	⬆️
molsym/salcs/nonstandard_frame.py	`89.87% <89.87%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sgoodlett

Also, It's not clear to me what D' and T' and why a pseudoinversion is necessary (other than the fact that you need the inverse of a non-square matrix). Can we use the square unitary matrices that define the basis rotation?

sgoodlett · 2026-08-25T15:34:41Z


+
+    @classmethod
+    def nonstandard_symtext(cls, symtext, max_degree=10):


This bit needs to just be a new function or we'll run into circular importing. As it stands, salcs imports from symtext so I think it's bad practice to import salcs from symtext too.

sgoodlett · 2026-08-25T15:55:50Z

+        intertwining residual for each operation.
+    :rtype: tuple(list, NumPy array)
+    """
+    Phi_prime = np.asarray(Phi_prime, dtype=float)


Phi_prime is already supposed to be a numpy array

sgoodlett · 2026-08-25T15:56:53Z

+
+# Small format-conversion helper: SALC objects -> a plain coefficient matrix
+# for the linear algebra in rotate_salcs/compute_dprime.
+def salc_to_phi(salcs, nfxn):


These functions: salc_to_phi and grouped_partner_sets can probably live in the SALC class since they only change the presentation of data already there.

sgoodlett · 2026-08-25T15:57:57Z

+
+# Carries a standard-frame SALC over to the nonstandard frame by an exact
+# change of coordinates -- no fitting, just re-expressing the same SALC.
+def rotate_salcs(Phi_std, Q, degree, tol=global_tol):


This function only seems to be invoked once, might be worth absorbing it into select_dprime_partner_sets

nlk36701 · 2026-08-26T12:33:36Z

Also, It's not clear to me what D' and T' and why a pseudoinversion is necessary (other than the fact that you need the inverse of a non-square matrix). Can we use the square unitary matrices that define the basis rotation?

T'(g) is the polynomial representation operator on the full function space (built from symel.rrep, size nfxn×nfxn) in the rotated/original molecular frame. D'(g) is the d×d irrep matrix, i.e. nonstandard_symtext.irrep_mats[symbol][g]. Phi' is the nfxn×d matrix embedding the SALC subspace into the full function space in that frame. They're related by T'(g)Phi' = Phi'D'(g). D' is just T' restricted to the invariant subspace Phi' spans, so solving for it needs Phi'⁺, and pinv is necessary because Phi' is rectangular (nfxn > d), not square.

On using the unitary basis-rotation matrix instead: that's already what rotate_salcs does (poly_rep(Q.T), an nfxn×nfxn operator) to carry Phi_std into Phi', but that operator is only orthogonal at degree 1. Past that, the raw monomial basis (x²,xy,...) isn't rotation-orthogonal (different monomials collapse different numbers of symmetric-tensor index arrangements), so Phi' itself isn't orthonormal and Phi'⁺ != Phi'ᵀ. Making it orthonormal would need a degree-aware normalization specific to PolynomialFunctions that ProjectionOp doesn't have a hook for today. We can add that... but it would make the PR bigger!

Q:
[[ 0.612 -0.176 0.771]
[ 0.666 -0.411 -0.623]
[ 0.427 0.894 -0.135]]

Using a fixed, arbitrary rotation Q (proper rotation, det=1), here's T·Tᵀ at each degree.

Degree 1 — basis (x, y, z):
[[ 1. 0. -0.]
[ 0. 1. -0.]
[-0. -0. 1.]]

Degree 2 — basis (x², xy, xz, y², yz, z²):

[[ 0.685 -0.092 -0.017 0.116 0.041 0.199]
[-0.092 1.465 0.081 0.255 -0.203 -0.163]
[-0.017 0.081 1.796 -0.102 -0.327 0.119]
[ 0.116 0.255 -0.102 0.835 -0.057 0.049]
[ 0.041 -0.203 -0.327 -0.057 1.197 0.017]
[ 0.199 -0.163 0.119 0.049 0.017 0.752]]

Degree 3 — basis (x³, x²y, x²z, xy², xyz, xz², y³, y²z, yz², z³):

[[ 0.4 -0.148 -0.034 0.18 0.095 0.359 0.011 -0.034 -0.07 0.011]
[-0.148 1.342 0.163 0.265 -0.424 -0.373 0.246 0.041 0.315 0.013]
[-0.034 0.163 1.961 -0.203 -0.837 0.203 0. 0.232 0.081 0.398]
[ 0.18 0.265 -0.203 1.688 0.025 0.165 0.492 -0.348 -0.207 -0.019]
[ 0.095 -0.424 -0.837 0.025 2.921 0.178 -0.198 -0.158 -0.2 -0.321]
[ 0.359 -0.373 0.203 0.165 0.178 2.144 -0.014 -0.16 -0.564 0.223]
[ 0.011 0.246 0. 0.492 -0.198 -0.014 0.673 -0.127 0.073 0.004]
[-0.034 0.041 0.232 -0.348 -0.158 -0.16 -0.127 1.102 -0.003 0.086]
[-0.07 0.315 0.081 -0.207 -0.2 -0.564 0.073 -0.003 1.059 0.007]
[ 0.011 0.013 0.398 -0.019 -0.321 0.223 0.004 0.086 0.007 0.508]]

max |T(Q)·T(Q)ᵀ − I| by degree:
degree 1: 0.000 (orthogonal — T(Q) is just Q)
degree 2: 0.796
degree 3: 1.921

sgoodlett · 2026-08-28T15:40:33Z

+                "Phi_prime": Phi_prime,
+            }
+
+            print(


We shouldn't print things unless requested

Copilot

Pull request overview

Adds support for constructing symmetry data in a molecule’s original orientation.

Changes:

Derives nonstandard-frame irrep matrices using spherical-harmonic representations.
Generalizes dense function-set and Cartesian SALC seed discovery.
Adds rotated fixtures and regression tests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`test/xyz/methane_rotated.xyz`	Adds rotated methane geometry.
`test/xyz/boric_acid_rotated.xyz`	Adds rotated boric-acid geometry.
`test/xyz/ammonia_rotated.xyz`	Adds rotated ammonia geometry.
`test/test_projection_op.py`	Preserves Psi4 ordering in existing tests.
`test/test_nonstandard_irrep_mats.py`	Tests transformed irrep matrices.
`test/test_nonstandard_cartesian_coordinates.py`	Tests dense Cartesian SALC discovery.
`molsym/symtext/symtext.py`	Adds original-orientation `Symtext` construction.
`molsym/salcs/spherical_harmonics.py`	Adds ordering support and dense shell functions.
`molsym/salcs/salc.py`	Adds partner-set matrix helpers.
`molsym/salcs/polynomial_functions.py`	Adopts shared dense-function behavior.
`molsym/salcs/nonstandard_frame.py`	Implements nonstandard irrep derivation.
`molsym/salcs/function_set.py`	Introduces `DenseFunctionSet`.
`molsym/salcs/cartesian_coordinates.py`	Adds dense-frame symmetry seed discovery.

Suppressed comments (1)

molsym/symtext/symtext.py:78

The deep copy retains assign_dipole_irrep, but that mapping was computed for the standard-frame Cartesian axes during Symtext.__init__. After rotating the molecule, operations, and irrep matrices, this cached frame-dependent mapping is stale and can report incorrect dipole components; recompute it before returning.

        nonstandard_symtext.irrep_mats = derive_nonstandard_irrep_mats(standard_symtext, nonstandard_symtext, max_degree=max_degree)
        nonstandard_symtext.is_nonstandard = True

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        for symel in nonstandard_symtext.symels:
+            R_std = np.array(symel.rrep, dtype=float)
+            symel.rrep = Q @ R_std @ Q.T



+
+    @classmethod
+    def nonstandard_symtext(cls, symtext, max_degree=10):


    FunctionSet for spherical harmonic basis functions.
    """
-    def __init__(self, symtext, fxn_list) -> None:
+    def __init__(self, symtext, fxn_list, order="increasing") -> None:


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

molsym/salcs/spherical_harmonics.py:190

Like generateRotations, rotate_em is listed in the generated public API and previously returned psi4-ordered matrices when called without an ordering. This default now silently changes existing callers' basis convention. Preserve the old default; new increasing-order callers already pass their order explicitly.

def rotate_em(maxam, ops, order="increasing"):

@@ -188,7 +205,12 @@ def reduction_coefficients(self, rrep_characters, by_class=True):
        for irrep_idx, irrep in enumerate(self.irreps):
            p = np.multiply(class_chars, self.class_orders)
            p = np.multiply(p, self.character_table[irrep_idx, :])


    return beeb

-def generateRotations(Lmax, rot):
+def generateRotations(Lmax, rot, order="increasing"):


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

molsym/salcs/nonstandard_frame.py:25

This conversion can be invoked again on an already nonstandard Symtext. In that case reverse_rotate is applied a second time and every operation is conjugated again, so the returned molecule is in a Q² frame rather than the original frame. Reject is_nonstandard inputs (or make the operation explicitly idempotent) before applying the transform.

    standard_symtext = symtext
    nonstandard_symtext = deepcopy(symtext)

    nonstandard_symtext.mol = symtext.mol.transform(symtext.reverse_rotate)

+        Phi = np.zeros((nfxn, len(salcs)))
+
+        for col, salc in enumerate(salcs):
+            coeffs = np.asarray(salc.coeffs, dtype=float)
+
+            if coeffs.shape[0] != nfxn:
+                raise ValueError(
+                    f"SALC coeff length {coeffs.shape[0]} does not match nfxn={nfxn}"
+                )
+
+            Phi[:, col] = coeffs


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

+    standard_symtext = symtext
+    nonstandard_symtext = deepcopy(symtext)


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

molsym/salcs/nonstandard_frame.py:32

The primary round-trip guarantee is not covered: none of the new tests compares nonstandard_symtext.mol.coords with the centered molecule coordinates supplied to Symtext.from_molecule. The representation/character and SALC-count assertions are self-consistency or frame-invariant checks, so a transpose/sign error in this rotation could still pass. Please retain the centered input molecule in a test and assert that this line reconstructs its coordinates (and ideally that each rotated rrep acts on that geometry as expected).

    nonstandard_symtext.mol = symtext.mol.transform(symtext.reverse_rotate)

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

+                    if new_rank > old_rank:
+                        symm_equiv.append([seed])
+                        current_basis = trial


…tion Symtext.nonstandard_symtext() rotates a symtext's operation matrices and molecule into its original (non-standard) orientation, then derives the correspondingly-rotated irrep matrices by rotating a known-correct standard-frame SALC into the new frame and solving D'(g) = Phi'^+ T'(g) Phi' directly, rather than deriving them from scratch. 1D irreps are copied directly instead of solved, since a 1x1 matrix is frame-invariant by definition -- this also sidesteps a real failure mode for complex point groups, where remove_complexity recombines conjugate 1D pairs into real vectors that are individually not group-invariant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ation matrices get_symmetry_equiv_functions() decides which Cartesian displacement coordinates need their own SALC-projection seed by checking which entries of the group's operation matrices are nonzero, using exactly one seed per merged set with no check that one seed actually suffices. That assumption only holds when the matrices are sparse -- true in the standard orientation, but not in a genuinely rotated (nonstandard) frame, where it silently undercounts SALCs (a random 3D rotation of boric acid: 15 SALCs found instead of the correct 21). Adds get_symmetry_equiv_functions_nonstandard(), auto-dispatched via symtext.is_nonstandard, which verifies by rank instead of assuming: it expands the frontier from every newly-touched coordinate (two coordinates can be genuinely different orbits that still overlap in support through a third coordinate) before selecting enough seeds within each merged component to actually span it. get_symmetry_equiv_functions() itself is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

test_nonstandard_irrep_mats.py: group-multiplication-table, orthogonality, and character-invariance checks for derive_nonstandard_irrep_mats across a real 2D irrep (ammonia E), a real 3D irrep (methane T1/T2), and a complex point group (boric acid); the 1D copy-through path; a RuntimeError on an unresolvable irrep; recovery from a deliberately-injected rank-deficient candidate; and a cross-check against the independent Schur's-lemma method where available (skipped otherwise). test_nonstandard_cartesian_coordinates.py: legacy/nonstandard agreement on an already-standard frame, SALC-count parity between standard and nonstandard frames (including the specific random-rotation boric acid case that originally exposed the undercount), a no-duplicate-seeds regression check, and Eckart-projected DOF counts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…otations Replace the homogeneous-polynomial + pseudoinverse approach in select_dprime_partner_sets with the exact orthogonal real spherical harmonic rotation matrices (sh_rep/SphericalHarmonicFunctions), so D'(g) comes from an exact orthogonal change of basis (Phi_prime.T @ T'(g) @ Phi_prime) instead of a least-squares fit -- escalating angular momentum l instead of polynomial degree. Also makes the spherical harmonic m_l ordering configurable (increasing/-l..l by default, psi4 order opt-in) instead of always resorting to psi4's convention, and factors the generic dense-fxn_map orbit-finding/projection logic shared by PolynomialFunctions and SphericalHarmonicFunctions into a new DenseFunctionSet base class. Validated against SymmetricForceFields end-to-end (psi4 HF/STO-3G finite difference Hessians/frequencies) for ammonia (C3v, E) and methane (Td, E/T_1/T_2), matching the standard-frame reference to FD precision with D' intertwining residuals at machine precision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…d subgroup symtexts and added regression tests.

…ult m_l order reduction_coefficients used the irrep character directly instead of its complex conjugate in the projection formula, so reducing a complex irrep's own character (e.g. C3h's E(1)') attributed the count to its conjugate partner (E(2)') instead -- confirmed and fixed per Copilot's review. Also revert generateRotations/rotate_em's default order back to "psi4" (Copilot flagged these, not just SphericalHarmonics, as documented public API whose omitted-order behavior shouldn't silently change); the new sh_rep/SphericalHarmonicFunctions still default to "increasing" and always pass it explicitly, so they're unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

build_nonstandard_symtext kept the original reverse_rotate on its output but never checked whether its input was already nonstandard, so calling it a second time silently applied Q again (landing in a Q^2 frame) instead of erroring -- confirmed empirically per Copilot's review. Use the existing is_nonstandard flag to reject this misuse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

build_nonstandard_symtext crammed a description into :type max_degree: instead of splitting it into :param:/:type: (the same issue already fixed in derive_nonstandard_irrep_mats), and Symtext.nonstandard_symtext's wrapper had no description at all. Made all three consistent, matching the codebase's established Sphinx convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Add test_nonstandard_symtext_mol_matches_original_geometry: the existing nonstandard-frame tests are all self-consistency or frame-invariant checks that would still pass under a transposed or sign-flipped Q, since everything downstream is rotated by that same wrong Q consistently. This compares against the original, COM-centered input geometry directly, per Copilot's review -- verified correct for all three test molecules before adding the assertion. Also trim dipole_active_irreps's docstring and SphericalHarmonicFunctions' class comment back down to this codebase's normal one-line-docstring, no-comparison-to-prior-approaches style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

nlk36701 force-pushed the nonstandard-irrep-mats branch from 57b3a1c to a3142f7 Compare August 24, 2026 22:06

sgoodlett requested changes Aug 25, 2026

View reviewed changes

Base automatically changed from more_tests to main August 25, 2026 17:13

nlk36701 force-pushed the nonstandard-irrep-mats branch from a3142f7 to 7abb779 Compare August 25, 2026 17:13

nlk36701 requested a review from sgoodlett August 28, 2026 01:28

sgoodlett reviewed Aug 28, 2026

View reviewed changes

nlk36701 requested a balanced review from Copilot August 28, 2026 23:31

Copilot started reviewing on behalf of nlk36701 August 28, 2026 23:32

View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

nlk36701 requested review from sgoodlett and a balanced review from Copilot August 29, 2026 17:25

Copilot started reviewing on behalf of nlk36701 August 29, 2026 17:26

View session