Repository navigation
feat: add diffpy.structure.Structure to pyobjcryst.Crystal converter - #13
Merged
Merged
Conversation
… to a pyobjcryst Crystal New module diffpy.cmistructure.converters. The atoms are reduced to the asymmetric unit with the Crystal's own space group, since ObjCryst and diffpy.structure pick different default origin choices (e.g. Fd-3m). Raises a ValueError if the symbol is unknown or the lattice or atoms don't fit the space group. Tests in tests/test_converters.py, fixtures in tests/conftest.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An int space group made pyobjcryst raise a Boost ArgumentError, so converters.py passes str(space_group) to Crystal. The unknown-space-group error now mentions numbers too. Tests in tests/test_converters.py cover 227 given as an int and as text, and 231. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 95.45% 96.08% +0.63%
==========================================
Files 5 6 +1
Lines 638 741 +103
==========================================
+ Hits 609 712 +103
Misses 29 29
🚀 New features to boost your workflow:
|
Contributor
Author
|
@sbillinge ready for review, please see "What should reviewers do?" section |
Contributor
Author
|
I know we talked about |
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.
What problem does this PR address?
There was no way to turn a diffpy.structure
Structureinto a pyobjcrystCrystal. srreal only converts the other way, and pyobjcryst only builds crystals from CIF files. This blocks anyone who has aStructurefrom a PDF fit and wants to use it in an ObjCryst calculation, such as a Rietveld fit.This PR adds
create_crystal_from_structure(structure, space_group)in a newdiffpy.cmistructure.convertersmodule. The name follows pyobjcryst'screate_crystal_from_cif.Structureusually holds the full cell in P1 and doesn't record its space group. Building theCrystalin P1 would silently make every lattice parameter and atom position refinable.Crystal's own symmetry operations. diffpy.structure and ObjCryst pick different default origin choices:"Fd-3m"is origin choice 1 in diffpy and origin choice 2 in ObjCryst.ValueErrorinstead of guessing, when:"P b n m","Pbnm","F d -3 m:1") or as a number (62or"62").Example 1: convert the full LaMnO3 cell
Example 2: a space group that doesn't fit
Tests: 41 pass locally, both with pyobjcryst
mainand with released pyobjcryst 2025.1.0 and diffpy.structure 3.5.0. The new tests are intests/test_converters.py, and the silicon and asymmetric-unit fixtures are intests/conftest.py.What should the reviewer(s) do?
Please review the code and give feedback on these choices:
diffpy.cmistructure. Exporting it would make importing the package also import pyobjcryst.