New Feature: Allow passing phase to sample generation

CSSFrancis commented 5 months ago

Description of the change

Adds #480 for sampling generators.

Progress of the PR

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

Minimal example of the bug fix or new feature

a = 5.431
latt = Lattice(a, a, a, 90, 90, 90)
atom_list = []
for coords in [[0, 0, 0], [0.5, 0, 0.5], [0, 0.5, 0.5], [0.5, 0.5, 0]]:
    x, y, z = coords[0], coords[1], coords[2]
    atom_list.append(
        Atom(atype="Si", xyz=[x, y, z], lattice=latt)
    )  # Motif part A
    atom_list.append(
        Atom(atype="Si", xyz=[x + 0.25, y + 0.25, z + 0.25], lattice=latt)
    )  # Motif part B
struct = Structure(atoms=atom_list, lattice=latt)
p = Phase(structure=struct, space_group=227)
rotations = get_sample_reduced_fundamental(resolution=4, phase=phase)

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.

hakonanes commented 5 months ago

Thanks for this, @CSSFrancis.

But, I suggest to wait with this until after the v0.12.0 release. As @harripj asked and I agreed with in #480, we should allow this all places where it makes sense.

Also, I'm not sure of the best implementation. Should we allow passing a Phase to the point_group parameter? What about to space_group parameter? Or should we replace these parameters with one which can be all these types?

CSSFrancis commented 5 months ago

@hakonanes That's fair! We can just make the 0.12.0 release and then figure this out later if you think that is a good idea.

I was just trying to follow what was there previously. We should probably have one parameter that checks if it is a point group, a space group or phase or something that checks to make sure only one is defined.

pyxem / orix