-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Discrepancy between Phase.from_cif and Phase(structure) #491
Conversation
Hi @dasilvaale, Well spotted! And great job creating a fix right away. As you show, removing the extra call to To record your contributions, would you be so kind as to add your name (or GitHub handle) in between Viljar and Alexander in the Lines 18 to 19 in 856491e
Lines 45 to 53 in 856491e
Since this is a bug fix, I think we should release a 0.12.1 patch release right away. Could you change the base branch to And, is it OK with you if I push to your branch to prepare it for release (after we merge)?
Setting the lattice base twice is actually not the issue. The issue in Building on your example, we can compare the two routes in Steps in from copy import deepcopy
structure2 = deepcopy(structure)
# 1
A_old = structure2.lattice.base
A_new = _new_structure_matrix_from_alignment(A_old, x="a", z="c*")
# 2
r_old = structure2.xyz_cartn
# 3
structure2.lattice.setLatBase(A_new)
# 4
structure2.xyz_cartn = r_old
assert not np.allclose(structure.xyz, structure2.xyz)
# True
assert np.allclose(structure.xyz_cartn, structure2.xyz_cartn)
# True Steps in structure3 = deepcopy(structure)
# 1
A_old1 = structure3.lattice.base
A_new1 = _new_structure_matrix_from_alignment(A_old1, x="a", z="c*")
# 2
structure3.lattice.setLatBase(A_new1)
# 3
A_old2 = structure3.lattice.base
A_new2 = _new_structure_matrix_from_alignment(A_old2, x="a", z="c*")
# 4
r_old = structure3.xyz_cartn
# 5
structure3.lattice.setLatBase(A_new2)
# 6
structure3.xyz_cartn = r_old
assert not np.allclose(structure.xyz, structure3.xyz)
# False
assert np.allclose(structure.xyz_cartn, structure3.xyz_cartn)
# False
# Structure matrix unchanged after setting it the second time
assert np.allclose(A_new1, A_new2)
# True Fixing steps in structure4 = deepcopy(structure)
# 1
A_old1 = structure4.lattice.base
A_new1 = _new_structure_matrix_from_alignment(A_old1, x="a", z="c*")
# 2
r_old = structure4.xyz_cartn
structure4.lattice.setLatBase(A_new1)
# 3
A_old2 = structure4.lattice.base
A_new2 = _new_structure_matrix_from_alignment(A_old2, x="a", z="c*")
# 4
#r_old = structure4.xyz_cartn
# 5
structure4.lattice.setLatBase(A_new2)
# 6
structure4.xyz_cartn = r_old
assert not np.allclose(structure.xyz, structure4.xyz)
# True
assert np.allclose(structure.xyz_cartn, structure4.xyz_cartn)
# True |
Hi @hakonanes thanks for looking into this and thanks for the explanations. I'm glad the fix makes sense and am happy to contribute. I have updated the branch to main and added my affiliations to the files you mentioned. And yes, please feel free to push to this branch. |
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
@dasilvaale, thank you for this contribution! It has now been published in orix 0.12.1 on both PyPI and Anaconda 🚀 |
Description of the change
Hello,
there appears to be a discrepancy in the atom xyz positions between creating a Phase using from_cif and assigning a structure directly (Phase(structure)).
This appears to be because lattice alignment is perfomed twice when using from_cif, once in the method itself and once in the Phase.structure setter.
I have removed the alignment in Phase.from_cif, as it will be called in the Phase constructor and added some checks to the test below, which were previously failing with the double lattice alignment.
Progress of the PR
Minimal example of the bug fix or new feature
For reviewers
__init__.py
.section in
CHANGELOG.rst
.__credits__
inorix/__init__.py
and in.zenodo.json
.