Repository navigation
feat: a CIF read warns when the operation loop and the Hall symbol disagree - #59
Merged
Merged
Conversation
HallSymbol.operations reads a Hall space-group symbol into every operation of the group in the setting it states (centring, generators, default axes, screw digits, origin shift; exact arithmetic in twelfths). All 530 settings spglib 2.8.0 lists read to exactly the operation set of spglib.get_symmetry_from_database (7 388 operations). CIFImport now uses the Hall symbol (_space_group_name_Hall or _symmetry_space_group_name_Hall): - no operation loop: the cell is expanded with the Hall group (was refused as missing operations). pymatgen-core 2026.10.16 looks the Hall symbol up among Hermann-Mauguin names, finds none and builds P1 with a warning. - a loop, and the group named by Hall symbol alone: a loop with fewer distinct operations than the Hall group is refused (incompleteSymmetryOperationsForSymbol, naming the Hall symbol); it was admitted (rock salt listing its 48 point operations built 2 atoms of 8). - a Hall group smaller than the IT number or Hermann-Mauguin symbol beside it is refused like a short loop. - a loop, even an identity-only one, still wins over the symbol. CIFDocument gains declaredHallSymbol. Pins: tools/pins/cif-symmetry/hall-pins.py writes hall-symbol-pins.json (parity row P37). Done: Hall-only CIFs read and checked. Next: check a listed operation list against the cell metric, its closure and the declared crystal class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A count cannot see wrong operations. After the count checks, the expanding list (a CIF loop, or the Hall group when there is no loop) must now - map the cell onto itself: every rotation keeps R^T G R = G within Policy.operationMetricTolerance (new, default 0.01, per entry relative to sqrt(G_ii G_jj)); - be closed modulo lattice translations (compared to 1e-3; a list whose translations all lie within 0.004 of twelfths is read in twelfths; lists above SymmetryDetection.closureCheckLimit are not checked); - have the crystal class of the IT number the file states (read from the rotation types, so in any setting). A failure throws CIFImportError.inconsistentSymmetryOperations(SymmetryListProblem), naming the operation, the missing product or the two classes. pymatgen-core 2026.10.16 checks none of these and expands the list as given. Diagnosis: Fm-3m listing its 48 conventional-basis operations in the primitive 60-degree cell passed the count (192 / 4) and was expanded; P4/mmm with one operation swapped built 16 atoms of a structure with no symmetry; P4/m declared with the Pmmm list built the Pmmm cell. All three are refused now; spglib groups pin the class table for all 530 settings. Tests that asserted the wrong-basis list was admitted now expect the refusal; a policy fixture whose list was not a group is now one; the absurd-cell test opens the metric check through the policy. BREAKING CHANGE: CIFImportError gains the case inconsistentSymmetryOperations (an exhaustive switch needs it). CIFImport.Policy gains operationMetricTolerance (defaulted, source compatible). Files whose operation list does not fit the cell, is not closed or has another crystal class than the IT number are refused where they were read before; the metric check can be relaxed through the policy. Done: wrong operation lists refused by name. Next: a closed list of the right class with the wrong translations stays unchecked without a Hall symbol (open-items). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs/parity.md gains a DEVIATION in the CIF symmetry row: pymatgen-core 2026.10.16 CifParser admits an operation list that does not map the cell onto itself, is not closed or has another crystal class than the IT number, and expands it as given; the kit refuses it and names the cause in words. A full loop that disagrees with the Hall symbol beside it is used, as pymatgen does. A test checks that every SymmetryListProblem says its cause in words (not mapping the cell onto itself, not a space group, the two crystal classes or none of the 32) and that the error an app shows carries that sentence whole. Done: refusals named in words and the deviation recorded. Next: a warning on the result when a full loop disagrees with the Hall symbol (needs a warnings field on CIFDocument, filed separately). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs/parity.md conflicted on the P37 row and the CIF symmetry row: main carries them as #48 left them, this branch extends both with the operation-list checks (OperationListCheckTests, the crystal-class pin, the refusal and its pymatgen DEVIATION). Both rows keep the branch text, which is main's plus the checks. Done: branch re-synced with main. Next: CI on the PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…beside it CIFDocument.warnings (CIFImportWarning.operationLoopIsNotTheHallGroup) names the Hall symbol and both orders when a listed loop is not the Hall group modulo lattice translations. The loop is still used, as pymatgen-core 2026.10.16 CifParser uses it. HallLoopWarningTests: a P4/mmm loop beside -P 2 2 warns, beside -P 4 2 or no Hall symbol it does not. Stacked on issue-9-symop-checks. Not pushed: the work was paused before review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A P4/mmm loop beside the Hall symbol of P4/mcc (-P 4 2c, also 16 operations) now warns in a test, so dropping the operation comparison and keeping only the order check turns the suite red (checked by that mutation). A Hall symbol that cannot be read beside a valid loop reads with no warning and no error. The warning and docs/parity.md say a loop that passes the completeness checks, not a full loop. Done: review findings on the warning fixed. Next: open the PR after #49. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs/parity.md: main adds the operation-list checks to the CIF symmetry row; this branch already carries that text and adds the Hall loop warning after it, so the row keeps the branch text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Requested by Paul · project thread
Fixes #50
Merge #49 first. This branch is stacked on
issue-9-symop-checks. Until #49 lands, this diff includes its commits.What it does
A CIF can list an operation loop and also state a Hall symbol (
_space_group_name_Hall) that belongs to another group. The read still uses the loop, as pymatgen-core 2026.10.16CifParserdoes (pymatgen does not read the Hall symbol). Now it also tells the caller:CIFImportWarning.operationLoopIsNotTheHallGroup(hallSymbol:loopOrder:hallOrder:):Sendable,Equatable, with a plain-wordsdescription.CIFDocument.warnings: [CIFImportWarning], empty when there is nothing to report.CIFDocumenthas no public initializer, so no consumer has to change.docs/parity.mdrecords the addition next to the pymatgen behaviour. The DocC topic list names the new type.Tests
HallLoopWarningTests:-P 2 2warns (orders 16 and 8);-P 4 2cwarns (both orders 16, different operations);-P 4 2, or with no Hall symbol, there is no warning;The equal-order case was checked by mutation: with the operation comparison removed, it fails.
Local gate:
swift test -c release --skip DiffractoKitParityTestspasses, 187 tests. A reviewer pass ran on the new public API. The follow-up commit fixes its findings: a missing test for equal orders, a missing test for an unreadable symbol, and a wording fix.🤖 Generated with Claude Code