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).

@@            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     

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbillinge ready for review

@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.

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.

@sbillinge Here is my general understanding of these files.

The files in the subpackage structure are:

basestructureparset.py  cctbxparset.py          objcrystparset.py       srrealparset.py
bvsrestraint.py         diffpyparset.py         sgconstraints.py

The file that gets used the most (I think) is sgconstraints.py because this holds the function constrain_as_space_group(). Otherwise, I have yet to see a cmi script that imports anything else from this subpackage.

These *parset.py files are structure wrappers to make them useable for srfit refinement.

What would be ideal is to move sgconstraints.py to diffpy.structure, however i dont think this is possible. sgconstraints.py is not dependent on diffpy.srreal (good), however, it is dependent on diffpy.srfit meaning diffpy.structure would have to be dependent on srfit (bad). After some brief discussion with claude, it seems not possible to put this in diffpy.structure without adding the srfit dependency because it directly links to srfit objects.

To your point about the hypothetical diffpy.cmirietveld, if we wanted to use the constrain_as_space_group() function in cmirietveld we'd have to import it from diffpy.cmipdf if we put it in here. This is an argument for a separate, lightweight diffpy.cmistructure package.

The pros of having diffpy.cmistructure: Everything becomes more modular
The cons: More maintenance

What do you think?

Good analysis, let me think about it a bit

I think that ideally srift wouldn't depend on structure and vice versa as in principle we want each to be used separately than the other. This meant that we would need the parameter adaptors to be elsewhere. The most logical place is presumably in cmipdf, cmirietveld etc. And we may need a cmistructure if it is used by both? But I am not sure. We could look in more folks too to discuss it?

@sbillinge Creating a diffpy.cmistructure is easy but feels a bit overkill just for structure adapters.

I think its okay to have it in cmipdf for now at least. If we made a cmirietveld it would likely have the same/similar dependencies as cmipdf assuming it uses srreal, meaning adding cmipdf as a dependency wouldn't be a big deal (although maybe not the cleanest). Later if we wanted to create a cmistructure package, it would be a pretty easy thing to do, too. We would just deprecate things here and create a diffpy.cmistructure package.

closing as we will put this in https://github.com/diffpy/diffpy.cmistructure