Repository navigation
change: let the set_structure method take a path, parameter set, or structure object - #20
Merged
Merged
Conversation
…e file path Tests in tests/test_generators.py for BasePDFGenerator and DebyePDFGenerator.set_structure accepting a diffpy.cmistructure ParameterSet (kept under its own name; a name given with it raises a ValueError) or a pathlib.Path/str path read with load_structure. Signatures (name now defaults to None) and docstrings are updated; the bodies are not implemented yet, so these tests and the existing ones relying on the default name fail until they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@sbillinge ready for review |
docformatter v1.7.7 depends on untokenize 0.1.1, which fails to build on Python 3.14 (now used by pre-commit.ci). v1.7.8 drops untokenize. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cadenmyers13
marked this pull request as draft
November 2, 2026 18:52
BasePDFGenerator.set_structure (and so DebyePDFGenerator's) now uses a diffpy.cmistructure ParameterSet as it is, raising a ValueError if a name is also given, and reads a pathlib.Path or str with diffpy.structure.load_structure. The default name "phase" now applies only to structure objects and paths. test_set_structure_with_parameter_set no longer reads generator.phase, which only exists when the ParameterSet is named "phase"; it checks the ParameterSet managed under its own name instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….cmipdf into set-structure-inputs # Conflicts: # src/diffpy/cmipdf/basepdfgenerator.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 97.05% 97.31% +0.26%
==========================================
Files 6 6
Lines 340 373 +33
==========================================
+ Hits 330 363 +33
Misses 10 10
🚀 New features to boost your workflow:
|
cadenmyers13
marked this pull request as ready for review
November 2, 2026 21:13
Contributor
Author
|
@sbillinge ready for review now |
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?
set_structurenow takes whichever form of structure the user has, either aPath,ParameterSet, or structure object. This essentially mergesset_structure_from_parsetintoset_structurewhich simplifies the code a bit. I found this slightly confusing when i first started writing cmi code.A structure object (unchanged):
A ParameterSet, used as it is, with its own name. Giving one ParameterSet to several generators shares the structure:
The path to a structure file, as a
Pathorstr, read withdiffpy.structure.load_structure:namewith a ParameterSet raises an error. A ParameterSet keeps its own name. Renaming it would rename it in every generator that shares it, sonamecan only be used with a structure or a path:namenow defaults toNone, soset_structurecan tell whether one was given. For a structure or a path, it still means"phase".set_structure_from_parsetis unchanged, so existing scripts keep working.