Add function to expand asymmetric unit cell of Phase

[viljarjf]

Description of the change

Adds Phase.expand_asymmetric_unit(), which adds all symmetrically equivalent Atom entries into Phase.structure.
This already exists in diffsims (ReciprocalLatticeVector.sanitize_phase(), with a #TODO to upsteam into orix. However, I don't think the implementation in diffsims is entirely correct, seeing as the discrepancy between diffsims' and diffpy's axis alignment convention does not seem to be accounted for.

This expansion seems to be performed automatically when reading from a cif which includes symmetry information. I added a test for this, which includes the cif-file test-cases as raw strings. If there is another preferred way to do this please let me know. I saw the cif_file-fixture, but I felt making these single-use files into fixtures was unnecessary.

Additionally, I found Structure.placeInLattice(), which does the same as #469 for both coordinates and ADPs.

Progress of the PR

• Docstrings for all functions
• Unit tests with pytest for all lines
• Clean code style by running black via pre-commit

Minimal example of the bug fix or new feature

>>> phase = Phase(
      structure=Structure(
          atoms = [Atom("Si", xyz=(0, 0, 1))],
          lattice=Lattice(4.04, 4.04, 4.04, 90, 90, 90)
      ),
      space_group=227,
  )
  >>> phase.structure
  [Si   0.000000 0.000000 1.000000 1.0000]
  >>> phase.expand_asymmetric_unit()
  >>> phase.structure
  [Si   0.000000 0.000000 0.000000 1.0000,
   Si   0.000000 0.500000 0.500000 1.0000,
   Si   0.500000 0.500000 0.000000 1.0000,
   Si   0.500000 0.000000 0.500000 1.0000,
   Si   0.750000 0.250000 0.750000 1.0000,
   Si   0.250000 0.250000 0.250000 1.0000,
   Si   0.250000 0.750000 0.750000 1.0000,
   Si   0.750000 0.750000 0.250000 1.0000]

For reviewers

• The PR title is short, concise, and will make sense 1 year later.
• New functions are imported in corresponding __init__.py.
• New features, API changes, and deprecations are mentioned in the unreleased
  section in CHANGELOG.rst.
• Contributor(s) are listed correctly in __credits__ in orix/__init__.py and in
  .zenodo.json.

[Copilot]

Pull Request Overview

This pull request adds a new method, expand_asymmetric_unit(), to the Phase class to expand its asymmetric unit by including all symmetrically equivalent atoms based on the specified space group. It additionally adjusts lattice alignment within the structure and imports the necessary diffpy components to support this functionality.

• Introduces expand_asymmetric_unit() in the Phase class.
• Adjusts lattice alignment in the structure setter using placeInLattice().
• Imports and utilizes ExpandAsymmetricUnit from diffpy for symmetry expansion.

Files not reviewed (1)

• CHANGELOG.rst: Language not supported

Comments suppressed due to low confidence (2)

orix/crystal_map/phase_list.py:390

• [nitpick] The variable name 'eau' is ambiguous. Consider renaming it to a more descriptive name such as 'expansionResult' or 'expandedData' to improve code clarity.

eau = ExpandAsymmetricUnit(self.space_group, xyz)

orix/crystal_map/phase_list.py:391

• The loop iterates over self.structure while a cleared copy (diffpy_structure) is being used to accumulate results. Verify that iterating over self.structure returns the intended original atom list, as this mixing may lead to unexpected behavior.

for atom, new_positions in zip(self.structure, eau.expandedpos):

[viljarjf]

It might be an idea to set the space group to P1 after expanding, to indicate that the symmetry has been accounted for. Maybe the function instead should be called something like expand_to_P1 like in cctbx? This would mess with the symmetry reduced zone though. From what I can think of, the only change adding the symmetrically equivalent atoms makes is to the multiplicity of the atoms, which I don't think is accounted for in the first place (which is why this PR exists). Leaving the space group as-is should be fine, I think. Any thoughts, @hakonanes or @CSSFrancis ?

[CSSFrancis]

@argerlt Any chance you can comment on this?

Should we be expanding this? Would it be better to have an attribute? I'm maybe not the best person to comment on this but I think we want to avoid transformations which mess with the symmetry reduced zone. I think it would be better to have one object.

@property
def asymetric_unit(self):
    """Return the expanded asymetric unit"""
    pass


@property
def symetric_unit(self):
    """Return the reduced symmetric unit"""
    pass

[argerlt]

Ignore my previous response, I didn't quite follow what @CSSFrancis was suggesting. better answer:

I think as this stands, this PR fine. I think overwriting the symmetry is potentially dangerous for the reasons @CSSFrancis said, and because functions that overwrite class attributes you don't expect them to can be frustrating to debug.

Building on that though, I think a function that changes any class attributes in an irreversible way can be potentially problematic, so I also like the idea of class functions that return the expanded or reduced unit, which I believe is what @CSSFrancis is suggesting.

If I were doing it, I think I would call the functions return_expanded unit and return_reduced_unit (though I think you only intend to write the first right now) and have them be properties that returned new Phase objects, but I leave that up to you.

Some other thoughts, take or leave them:

1. It would be nice to add an option in Phase.__init__ to auto-apply this expansion this exact problem has annoyed me in the past, and being able to expand during initialization would have saved me a half-day of futzing around.
2. I think there is an effort to use copy in place of deepcopy as part of Improve object copying #535
3. The tests you ran only cover cubic and hexagonal systems. it might be helpful to add something like monoclinic quarts to potentially catch future edge cases.