Skip to content

Fix CIF rhombohedral symmetry scoring mutating imported lattice#70

Open
clemisch wants to merge 3 commits into
vincefn:masterfrom
clemisch:cif_import_fix
Open

Fix CIF rhombohedral symmetry scoring mutating imported lattice#70
clemisch wants to merge 3 commits into
vincefn:masterfrom
clemisch:cif_import_fix

Conversation

@clemisch

@clemisch clemisch commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Score alternate CIF space-group settings on a temporary Crystal instead of the live imported object, preventing :R/:H probing from corrupting rhombohedral hexagonal cell metrics during pyobjcryst CIF import.

See #67

Score alternate CIF space-group settings on a temporary Crystal instead of the live imported object, preventing :R/:H probing from corrupting
rhombohedral hexagonal cell metrics during pyobjcryst CIF import.

happened with "_symmetry_equiv_pos_as_xyz" keyword in CIFs
@vincefn

vincefn commented Mar 30, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR - I'm not sure creating a tmp Crystal object is the best approach. It creates a new entry in the global Crystal registry, which should not be necessary.

Could this be done instead by just creating a tmp SpaceGroup object, or even better only using a sgtbx call ?

@clemisch

clemisch commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Updated the fix to avoid constructing a temporary Crystal during _symmetry_equiv_pos_as_xyz scoring. The candidate setting is now evaluated directly with cctbx::sgtbx::space_group. The live Crystal is only changed once at the end with the selected canonical symbol.

Is that what you meant in your suggestion?

One caveat: the candidate-parse path now uses catch(exception) only to preserve the old "skip invalid candidate setting" behavior with the sgtbx parser. That is broader than ideal and should be narrowed to the specific cctbx/sgtbx exception type.

@clemisch

clemisch commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

I think I found the appropriate exception type: cctbx::error

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