refactor: small interface and style improvements for constraint handling

[ycexiao]

This is a collection of small issues related to handling constraints in diffpy.srfit.

---

Problem

In SpaceGroupParameters, self._makeConstraints() is only called in __iter__() which means the parameters will not be set up properly by:

sgpars = constrainAsSpaceGroup(phase, "Fm-3m")

The following lines are required

for par in sgpars.latpars:
    pass  # usually recipe.addVar(par)
for par in sgpars.adppars:
    pass  # usually recipe.addVar(par) 

Proposed solution

Move self._makeConstraints() to __init__()

---

Problem

When imposing space group symmetry, if there exists isotropic ADPs, isotropic ADPs are handled but others are not. e.g.

sgpars = constrainAsSpaceGroup(ni_phase, "Fm-3m")
print(pdf_generator.phase.Ni0.Uiso.constrained)  # False as expected
print(pdf_generator.phase.Ni1.Uiso.constrained)  # True as expected

print(pdf_generator.phase.Ni0.U11.constrained)  # False, but Ni0.U11 should equal to Ni0.Uiso
print(pdf_generator.phase.Ni1.U11.constrained)  # False, but Ni1.U11 should equal to Ni1.Uiso equal to Ni0.Uiso

This is because diffpy.srfit intends to use Uiso to handle isotropic ADP and use Uij to handle anisotropic ADP. Both variables are created but only the ones that it intends to use are constrained correctly.

Proposed solution

I think _constrain_adps is part of the core functionalities and has been heavily tested so I don't want to change it. Maybe we could allow SpaceGroupParameters._make_constraints() or create a new method to return the variable g = SymmetryConstraints(sg, positions, Uijs, sgoffset=sgoffset), so that the downstream app can interact this part of information directly?

[sbillinge]

@ycexiao this seems like a pretty ugly fix. Is there something else wrong in our architecture that is causing this? I suggest trying to work with Claude to try and find a solution rather than this that looks like a workaround?

[ycexiao]

@sbillinge

this seems like a pretty ugly fix.

I want to make sure I understand your words correctly.

I think what you considered 'ugly' is that for the current implementation constraints are set only after __iter__ method are invoked and a less 'ugly' implementation is that constraints are set once the parameters are initiated via

sgpars = constrainAsSpaceGroup(phase, "Fm-3m")

Constraints are set via _self.makeConstraints() method. In the current architecture that method is only called in the __iter__ method. So the solution proposed here is to also call _self.makeConstraints() in the __init__ method because all required arguments are already set inside __init__

Usually users will find parameters only through the iteration so there is no problems right now. But if we can finish preparing the constraints in the __init__ method, the downstream apps can have a cleaner implementation.

[sbillinge]

I may have misunderstood. I thought that when you said "Move self._makeConstraints() to init" you were referring to moving it to the __init__.py and make it global, but I realize now you meant moving it to the __init__() method, the class constructor. I apologise. This latter understanding is actually fine.

[ycexiao]

I see. It is also my fault not making a clear distinction . The issue title and body is modified now.

[sbillinge]

no worries, I was also reading a bit too quickly.

[sbillinge]

in any case, please go ahead with this fix as you suggested.

[ycexiao]

@sbillinge, I also find some styling issues related to constraints. Is it suitable to also put them here? I can modify the title accordingly. Because it seems to me these modifications are too minor for separate issues/PRs.

[sbillinge]

Yes, this is fine

[ycexiao]

@sbillinge, could you please the new interface problem that associates with _constrain_adps? It is added in the issue body.

[sbillinge]

Thanks @ycexiao. I am not sure I completely understand your proposed solution. I am not sure how srfit handles the adps under the hood. In principle we only need U_ij for 1 < i,j < 3 and the U_iso is just a convenience function. I think U_iso goes to 3 x U_11 or something like that, not exactly U_iso = 3 x U_11 but that is a detail we can work out. In any case, could you ask your question more directly? I kind of understand the words in the issue description but I don't fully understand the question. Also, you calling a function in the example, so I don't fully understand why it matters if something is initialized or not.