Skip to content

feat: Add structure subpackage - #4

Open
cadenmyers13 wants to merge 3 commits into
diffpy:mainfrom
cadenmyers13:add-structure-subpackage
Open

cadenmyers13 wants to merge 3 commits into
diffpy:mainfrom
cadenmyers13:add-structure-subpackage

Conversation

@cadenmyers13

Copy link
Copy Markdown
Contributor

The diffpy.srfit.structure subpackage is dependent on the packages we are trying to eliminate from srfit, so I added it to this package.

Because getValue and addParameter were not deprecated on the last srfit release they remain camelcase for now. This will have to be updated upon the next srfit release

cadenmyers13 and others added 3 commits September 16, 2026 09:57
Port all 8 modules of diffpy.srfit.structure into diffpy.cmipdf.structure
so cmipdf owns the structure adapters it depends on, and repoint
basepdfgenerator at the local copy instead of diffpy.srfit.structure.

Per the deprecation plan, cmipdf has never been released and so carries
no deprecation shims: constrainAsSpaceGroup and the three setConst
forwarders are dropped along with the diffpy.utils._deprecator imports,
and the camelCase API is ported as snake_case only. getValue and
addParameter stay camelCase because they override core srfit methods
that have not been renamed yet; struToParameterSet keeps its name
because srfit is not renaming it.

Also add extend_path to src/diffpy/__init__.py, without which
diffpy.cmipdf shadows the rest of the diffpy namespace.

Tests ported from srfit for diffpyparset, objcrystparset and
sgconstraints, minus the constrainAsSpaceGroup deprecation test, plus
the LaMnO3.stru fixture and a pyobjcryst_available gate. Suite goes
from 12 to 29 passing, matching srfit's 18 for the same modules less
the one deprecation test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.86957% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.41%. Comparing base (c7bdc5f) to head (8748233).

Files with missing lines Patch % Lines
tests/test_objcrystparset.py 90.03% 33 Missing ⚠️
tests/test_sgconstraints.py 97.31% 4 Missing ⚠️
tests/conftest.py 75.00% 3 Missing ⚠️
tests/test_diffpyparset.py 98.79% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
+ Coverage   87.11%   91.41%   +4.30%     
==========================================
  Files           5        8       +3     
  Lines         194      769     +575     
==========================================
+ Hits          169      703     +534     
- Misses         25       66      +41     
Files with missing lines Coverage Δ
tests/test_diffpyparset.py 98.79% <98.79%> (ø)
tests/conftest.py 56.25% <75.00%> (+4.32%) ⬆️
tests/test_sgconstraints.py 97.31% <97.31%> (ø)
tests/test_objcrystparset.py 90.03% <90.03%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cadenmyers13

Copy link
Copy Markdown
Contributor Author

@sbillinge ready for review

@sbillinge sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand what is going on here. What are the design decisions that have been made. Can we have a quick conversation about what the high level design is and make sure that this is what we want to be doing?

@cadenmyers13

Copy link
Copy Markdown
Contributor Author

@sbillinge diffpy.srfit houses a subpackage called diffpy.srfit.structure. The code within this is dependent on diffpy.structure and diffpy.srreal which we are trying to remove as a dependency from srfit (removing diffpy.structure is not as big of a deal, but srreal for sure). Because the srfit.structure subpackage is used for cmi PDF refinement and this package is already dependent on diffpy.structure and diffpy.srreal I added it here.

In my opinion, we could either (1) add it here as a subpackage (diffpy.srfit.structure goes to diffpy.cmipdf.structure) or (2) break this out into its own package diffpy.cmistructure. This PR goes for option 1.

@sbillinge

Copy link
Copy Markdown
Contributor

I guess my question is what we can move from diffpy.cmipdf.structure into diffpy.structure and what we can't, and the same question for diffpy.srfit.structure. If I want to fit a Gaussian (so nothing to do with structure) I kind of don't need to know anything about structure. If I want to fit a PDF, I need to know about structure, so I will install the PDF pack, which will include diffpy.structure and diffpy.cmipdf. If I want to do rietveld refinement, I need to know about the same structure stuff, and also powder diffraction, so I would install the "rietveld pack" which would include diffpy.structure and diffpy.cmirietveld or whatever. Everything to do with structure I would like to import from diffpy.structure and everything about PDF I would import from diffpy.cmipdf and so on. There may be too much to unravel here, but let's at least try and only give up when we can't make it work.

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