Ensure atom positions are expressed in the new basis when initializing Phase with structure

viljarjf commented 6 months ago

Fixes (part of) https://github.com/pyxem/diffsims/issues/203

Description of the change

When initializing a Phase with a diffpy.structure.Structure, the lattice is transformed to orix's standard coordinate system. However, the atoms are still expressed as if they were in the old lattice. By taking the cartesian positions of the atoms before changing the lattice, and using those to set the new positions, the change of basis is accounted for.

All tests pass, but I don't think this was tested for previously. It probably should.

Progress of the PR

[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

from diffpy.structure import lattice, atom, Structure
from orix.crystal_map import Phase
import numpy as np

# Graphite
latt = lattice.Lattice(2.464, 2.464, 6.711, 90, 90, 120)
atoms = [atom.Atom(atype='C', xyz=[0.0, 0.0, 0.25], lattice=latt),
         atom.Atom(atype='C', xyz=[0.0, 0.0, 0.75], lattice=latt),
         atom.Atom(atype='C', xyz=[1/3, 2/3, 0.25], lattice=latt),
         atom.Atom(atype='C', xyz=[2/3, 1/3, 0.75], lattice=latt),
         ]
structure = Structure(atoms=atoms, lattice=latt)

phase = Phase("Graphite", point_group="6/mmm",  structure=structure)

# The lattice has been transformed
assert not np.allclose(structure.lattice.base, phase.structure.lattice.base)

for i in range(len(atoms)):
    print("xyz-basis:  ", structure[i])
    print("phase-basis:", phase.structure[i])
    print()

Output before:

xyz-basis:   C    0.000000 0.000000 0.250000 1.0000
phase-basis: C    0.000000 0.000000 0.250000 1.0000

xyz-basis:   C    0.000000 0.000000 0.750000 1.0000
phase-basis: C    0.000000 0.000000 0.750000 1.0000

xyz-basis:   C    0.333333 0.666667 0.250000 1.0000
phase-basis: C    0.333333 0.666667 0.250000 1.0000

xyz-basis:   C    0.666667 0.333333 0.750000 1.0000
phase-basis: C    0.666667 0.333333 0.750000 1.0000

Output with this PR:

xyz-basis:   C    0.000000 0.000000 0.250000 1.0000
phase-basis: C    0.000000 0.000000 0.250000 1.0000

xyz-basis:   C    0.000000 0.000000 0.750000 1.0000
phase-basis: C    0.000000 0.000000 0.750000 1.0000

xyz-basis:   C    0.333333 0.666667 0.250000 1.0000
phase-basis: C    0.577350 0.577350 0.250000 1.0000

xyz-basis:   C    0.666667 0.333333 0.750000 1.0000
phase-basis: C    0.577350 -0.000000 0.750000 1.0000

For reviewers

[x] The PR title is short, concise, and will make sense 1 year later.
[x] New functions are imported in corresponding __init__.py.
[x] 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.

pc494 commented 6 months ago

@viljarjf would you be comfortable writing a test case for this? If so, I'll wait for that and then get my crystallography hat on to check this works as it needs to.

CSSFrancis commented 6 months ago

@viljarjf I would just add the example in as a test. It at least makes sure that we explicitly know what basis we are working in and if for some reason diffpy changes then we would know.

viljarjf commented 6 months ago

I added a test now, a cubic lattice passes with the old code but the two non-cubic lattices failed. All three pass with the fix

CSSFrancis commented 6 months ago

@viljarjf Just a note that you need to run both black and isort, let me know if you need help doing that. @pc494 do you want to allow pre-commit similar to pyxem so we can just auto-fix files. I think that is much easier.

As far as the failing tests go. They seem unrelated. @pc494 You can rerun the tests if you would like and it might allow them to pass otherwise there is a different bug.

pc494 commented 6 months ago

Both,

I'm going to check this out today to refactor the testing a little bit and I'll get isort + black etc run then as well.

CSSFrancis commented 6 months ago

@pc494 This needs to be added to the Changelog as well

pc494 commented 6 months ago

pre-commit.ci autofix

CSSFrancis commented 6 months ago

@hakonanes I tested this vs Kikchipy and the tests appear to be passing.

CSSFrancis commented 6 months ago

@pc494 any idea why the documentation is failing?

pc494 commented 6 months ago

I'm going to tidy this up with a view to merge later today. Hopefully, the docs will figure themselves out,..

pc494 commented 6 months ago

pre-commit.ci autofix

pc494 commented 6 months ago

Given the current state of play, I am going to merge this, with new issues raised about both the RTD build and the contributors list.

pyxem / orix