Skip to content

Fix CIF rhombohedral symmetry scoring mutating imported lattice#70

Open
clemisch wants to merge 3 commits intovincefn:masterfrom
clemisch:cif_import_fix
Open

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

Conversation

@clemisch
Copy link
Copy Markdown
Contributor

@clemisch clemisch commented Mar 26, 2026

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
Copy link
Copy Markdown
Owner

vincefn commented Mar 30, 2026

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
Copy link
Copy Markdown
Contributor Author

clemisch commented Mar 30, 2026

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
Copy link
Copy Markdown
Contributor Author

clemisch commented Mar 30, 2026

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