Fix simulations of rotated structure

[pc494]

---

name: Fix simulations of rotated structure
about: Following on from #46, #56 , #57 and #59 this PR attempts to solve our rotations problems

---

Release Notes

major
improvement
Summary: Rotations of structures is changed.

What does this PR do? Please describe and/or link to an open issue.
Previously, .calculate_ed_data() took a structure rotate to the correct orientation. It appears that doing this with diffpy (rather than pymatgen as in old-pyxem) is problematic. Also theoretically it's a bit of a challenge. A cleaner option seems to be to calculate all reciprocal lattice points in the standard orientation - ie .reciprocal() of the standard lattice & then at the output stage rotate the cartesian coordinates.

An obvious advantage is illustrate by a 90 degree z-axis rotation of a cubic structure with (h00) types absent. In the old layout, it's hard to keep track of which is 'a' and which is 'b' - here we have two aligned arrays
index = [[1,0,0],[0,1,0]]
and the coordinates - for rotation (0,0,0) the cartesian_coordinates = [[a*,0,0],[0,b*,0]]
for rotation (0,0,90) we get [[0,a*,0],[-b*,0,0]]

Describe alternatives you've considered
Lots, none of which really worked.

Are there any known issues? Do you need help?

• Ideally @EirikOpheim would run this for us to check.

Work in progress?
If you know there is more to do, make a checklist here:

• Coverage dropped, probably due to simulate_rotated_structure() going, which can now be deleted.
• docstrings need checking

[pc494]

To get this PR locally use (Step 1 in the command line instructions box on the PR)

git checkout -b pc494-fix-simulate-rotate-structure master
git pull https://github.com/pc494/diffsims.git fix-simulate-rotate-structure

Then install this version of diffisms:
pip install .
(from inside the diffsims directory)

[pc494]

MWE test case.

import diffpy
from matplotlib import pyplot as plt
from diffsims.generators.diffraction_generator import DiffractionGenerator


def ortho_structure():
    """An atomic structure represented using diffpy, specifically a C lattice type
    which should have absences 
    """
    latt = diffpy.structure.lattice.Lattice(2,2.2,2.4,90,90,90)
    atom  = diffpy.structure.atom.Atom(atype='Ni',xyz=[0,0,0],lattice=latt)
    atom2 = diffpy.structure.atom.Atom(atype='Ni',xyz=[0.5,0.5,0],lattice=latt)
    structure = diffpy.structure.Structure(atoms=[atom,atom2],lattice=latt)
    return structure


ed = DiffractionGenerator(300,0.01)

def plot(sim_core):
    plt.figure()
    sim_core.calibration = (9e-3,9e-3)
    plt.imshow(sim_core.get_diffraction_pattern(),vmax=1e-2)
    
sim_core = ed.calculate_ed_data(ortho_structure(),3)
plot(sim_core)

sim_core = ed.calculate_ed_data(ortho_structure(),3,rotation=(90,0,0))
plot(sim_core)

sim_core = ed.calculate_ed_data(ortho_structure(),3,rotation=(-30,60,0))
plot(sim_core)

sim_core = ed.calculate_ed_data(ortho_structure(),3,rotation=(60,60,0))
plot(sim_core)

[dnjohnstone]

@EirikOpheim this should be easier for you to test after this merge - please do let us know if there are further issues.

[EirikOpheim]

Thanks Duncan,

I have been struggeling to get git working properly the last couple of days, but Håkon helped me yesterday, so I guess I should be able to check out branches soon. Now that it is included in the master branch I can run the tests. I will let you know how it goes.

Eirik

[EirikOpheim]

I run into problems with the function peaks_from_best_vector_match() in pyxem.indexation_utils() as this function also depends on the removed function "Simulate_rotated_structure()". Do we have to implement similar changes for this function? Is there a way to just ignore this dependency, while I am testing the changes to diffsims?

[dnjohnstone]

I run into problems with the function peaks_from_best_vector_match() in pyxem.indexation_utils() as this function also depends on the removed function "Simulate_rotated_structure()". Do we have to implement similar changes for this function? Is there a way to just ignore this dependency, while I am testing the changes to diffsims?

Can you be more specific please? Probably yes we'll need to introduce similar changes for that - please raise a bug report issue for this. What issue is it causing you for this?

[EirikOpheim]

Sure, I am sorry for writing a bit vague. The issue is that I can no longer import pyxem, after updating diffsims. I get " ImportError: cannot import name 'simulate_rotated_structure' from 'diffsims.utils.sim_utils' ". The function 'simulate_rotated_structure' was removed in this bugfix. The only place in sim_utils where simulate_rotated_structure is used is in the function peaks_from_best_vector_match().

I am not using this function in my notebook, but I believe that users who update diffsims will not be able to import pyxem with the current version. (I also updated pyxem to make sure I had the newest version.)

I can raise a bug report for this problem in pyxem.

[dnjohnstone]

Oh right, yeah that'll be an issue for pyxem - but also easy to fix by updating the imports in pyxem, do you know how to do that? A PR fixing it would be better than an issue reporting it if possible?

[EirikOpheim]

I am not quite sure how to do that, I am sorry! What we want to do is to change the function peaks_from_best_vector_match to no longer depend on simulate_rotated structure, and then remove the import, right?

I will go through some tutorials on git today, so that hopefully I can contribute more with PR's and stuff in the coming months.

Maybe it is best if you or Philip could make the PR for this issue? :)

[dnjohnstone]

Yes that's what needs to happen - just make a branch, make the changes, commit, push, PR - the same as when you did it before?

Since you're not using the function - in your local copy of pyxem you can just remove the import in the short term to let you move forwards.

I might be able to get to this on Monday.

[EirikOpheim]

The first time I made changes I did so on a branch in github.com and then pushed it, but I guess I could do the same thing again.

I will remove the function in my local copy and go on with the testing, and then try and implement the same changes in a pyxem PR.

Have a nice weekend!

[dnjohnstone]

Ok - I think if you intend to contribute code to pyxem over the course of your project (as discussed elsewhere) then you need to learn this basic git/github workflow sooner rather than later.

There is a little info here: https://github.com/pyxem/starter-pack

Are you working on Windows or Mac? If so, I'd recommend you use Github Desktop (https://desktop.github.com/)

Since this particular bug is a fairly simple example and one that you've found yourself - it's kind of an ideal first contribution. So if contributing is an important project goal for you I think it would be good for you to fix this.

Does that sound ok?

[EirikOpheim]

I agree that I should learn tu use github workflow as soon as possible. I am working on ubuntu, so it should not be too hard, I think. I had some issues relating downloaded packages from git to anaconda, but I am sure I will figure it out!

It sounds ok to me that I should perform the bugfix :)

[EirikOpheim]

My test notebook shows that the result is now consistent with the orix tool for handling symmetry. Well done @pc494!

[dnjohnstone]

My test notebook shows that the result is now consistent with the orix tool for handling symmetry. Well done @pc494!

Great! Is have you checked that for a few crystal systems? We need to be sure it's not just cubic cases for example!

[EirikOpheim]

I checked for the monoclinic crystal system now, using this cif file to describe the structure:
9016521.txt (Converted to .txt to allow upload to git, obtained from crystallography.net).

For a some reason indexer.correlate() gave an error message related to kwargs, allthough no keyword arguments were given. I thought I'd mention this, as I do not 100% trust the results I got, although they look sensible.

To work around the error I copied the indexer.correlate function to my notebook and removed the kwarg input as follows: (Copied from the indexer.correlate and removed kwargs where they appear)

from pyxem.utils.indexation_utils import correlate_library
from pyxem.signals.indexation_results import TemplateMatchingResults
from pyxem.signals import transfer_navigation_axes

library = template_library
signal = test_data

no_extra_methods_yet = True
if no_extra_methods_yet:
    # adds a normalisation to library
    for phase in library.keys():
        norm_array = np.ones(library[phase]['intensities'].shape[0])  # will store the norms
        for i, intensity_array in enumerate(library[phase]['intensities']):
            norm_array[i] = np.linalg.norm(intensity_array)
        library[phase]['pattern_norms'] = norm_array  # puts this normalisation into the library

    matches = signal.map(correlate_library,
                        library=library,
                        n_largest=3,
                        mask=False,
                        inplace=False)

matching_results = TemplateMatchingResults(matches)
matching_results = transfer_navigation_axes(matching_results, signal)

With these matching_results and using the orix symmetry C2h I get the following image:

It looks like something is wrong here, but I am not sure if it is related to the rotation of structures or something else. The lines of code written above ran exceptionally fast, so I suspect that something might have gone wrong, as usually indexer.correlate takes around 5 minutes to run and this ran in 30 seconds.

[pc494]

For a some reason indexer.correlate() gave an error message related to kwargs, allthough no keyword arguments were given. I thought I'd mention this, as I do not 100% trust the results I got, although they look sensible.

This should be raised as a bug within pyxem. MWE* + a stacktrace (ie - copy paste the error message) should help us figure it out.

It looks like something is wrong here, but I am not sure if it is related to the rotation of structures or something else. The lines of code written above ran exceptionally fast, so I suspect that something might have gone wrong, as usually indexer.correlate takes around 5 minutes to run and this ran in 30 seconds.

Starting at the top of this problem:
i) Am I right to think that the "data" is kinematical simulations?
ii) Which functionality did you use to generate the rotation list you've used here?
iii) Are the incorrect indexations all off by exactly 90 degrees (right hand figure)?

*MWE == Minimum Working Example,
The problem replicated in the fewest lines of code (so that it can be reproduced by another person)

[EirikOpheim]

This should be raised as a bug within pyxem. MWE* + a stacktrace (ie - copy paste the error message) should help us figure it out.

I can raise a bug in the pyxem forum with a MWE and a stacktrace.

Starting at the top of this problem:
i) Am I right to think that the "data" is kinematical simulations?
ii) Which functionality did you use to generate the rotation list you've used here?
iii) Are the incorrect indexations all off by exactly 90 degrees (right hand figure)?

1. You are right to think that data is kinematical simulations.
2. The rotation list was generated by structure_library = StructureLibrary.from_orientation_lists(['Au'],[au],[Angles]). I called the structure au when I imported from the cif file, to reuse the rest of the script. Angles is the list of angles used to in the kinematical simulations.
3. All incorrect indexations are off by exactly 90 degrees.

[pc494]

Point (3) reminds me of pyxem/orix#18 - have you tried plotting plots in the style of the left hand one (ie - distance to (0,0,0)) for both "true" and "matched" and visually comparing?

[EirikOpheim]

Good idea! I will try!

[EirikOpheim]

This is the true value. (Left and right are the same image)

[pc494]

This is the true value. (Left and right are the same image)

I think this is excellent news and more or less closes the diffsims component of this problem? I'll leave you to open the upstream pyxem issue and the downstream orix issue is already covered by pyxem/orix#18)

[EirikOpheim]

I think this is excellent news and more or less closes the diffsims component of this problem? I'll leave you to open the upstream pyxem issue and the downstream orix issue is already covered by pyxem/orix#18)

I am sorry, I think my comment was not so clear. I have just plotted the same image twice, it should be compared to the left image of

[pc494]

Thanks @EirikOpheim, I'm back up to speed. Would you be able to email me the notebook you're working from here?

[EirikOpheim]

Sure!

[EirikOpheim]

I found a bug in the code I have been using for testing with a new crystal system, so the plots I have posted today should be ignored.

[EirikOpheim]

I have some new results to share regarding testing of this function. First of all, I have tested for a orthorombic, non-cubic system. And the results were good.

I believe this bugfix works when all sides are different. For the example above I used a = 1, b=2 c = 10.

I have been using a reciprocal radius of 4, which I realize is quite big. I therefore tested how things work if I reduce this value. I believe this is not related to the functionality discussed in this pull request, but of interest in the case of the correlate function. I thought I'd present the results here, as they might be relevant for my results regarding the monoclinic system, and maybe we can proceed with the discussion in another thread.

For reciprocal radius: 2 (still a = 1, b= 2, c = 10)

Reciprocal radius: 1

I believe this suggests that the template matching method relies heavily on going far enough out in the reciprocal space for my test cases.

The reason why I wanted to bring that up is that for the monoclinic system I suspect the reciprocal radius might not be large enough. Here I have used lattice parameters (5.679, 15.202, 6.522, 90, 118.43, 90) (Found these parameters for Gypsum).
The result of my test is:

Which looks quite messy, and is very far off the correct result.

I have looked at a few simulated diffraction patterns, and seen that for Euler angles (a,b,0) and (a+180,-b,0) the simulated patterns are equal. I believe this is as it should be for all structures. Furthermore, an increase of the first Euler angle by n degrees will rotate the template n degrees clockwise. (I believe this is also as it should be).

To sum up, I believe that this bugfix works well for all orthorombic systems. I have not been able to confirm that it is working for the monoclinic system, as the test result is very messy, but the issue could lie elsewhere.

[dnjohnstone]

Thanks for this @EirikOpheim - I think that this suggests the code at least may now be correct and that remaining issues are wider project work - so I'll send you an e-mail about that.