FFT based kinematic simulator #33

robtovey · 2019-11-05T14:36:27Z

I have added a new branch called, for lack of a better name, 'kinematic_simulator_rob' which provides an FFT based simulation of diffraction images which follows an Ewald Sphere model. This hopes to address the enhancement request of #16. Coverage is incomplete but I will continue to add as other issues are dealt with. There is a small hole in the functionality in that I have only implemented a test version of a probe function but I will try to correct that in a minute...

Broad structure of the new code:

generators.diffraction_generator has a new object AtomicDiffractionGenerator which is intended as the front-end interface
sims.kinematic_simulation provides code which, essentially, converts a discretised volume into a diffraction pattern
utils.discretise_utils provides code (cpu and cuda) for discretising a volume with atoms inside it
utils.fourier_transform provides some wrappers of various fft codes, automatically selecting pyfftw if available

Some house-keeping questions:

I have assumed all units are angstroms, do I need to convert at any point?
Documentation in diffsims.diffraction_generator suggests that energy has unity kV, should this be keV?
In the code I adapted from I defined my floating precision basically globally but I'm not sure what default to use here. Could it be assumed that everyone uses float32 (or float64)? If not, I think either it should be a user parameter or automatically guessed from one of the inputs.
Current convention is a Fourier transform with no factor of pi. Is there a global convention for this or should it be a user preference?
When discretising an atom there is a cut-off radius which, currently just rounds anything smaller than e^{-20} to 0. Again, should this be a user parameter?
Is there a current convention for mesh vectors? i.e. if x, y are the two coordinate vectors then is x == x.reshape(-1) or x == x.reshape(-1, 1) or something else?

Other things I need to do:

Implement a generic probe class
Add appropriate documentation when a probe is an input parameter

p.s. I apologise for my editor's rather religious attitude when it comes to upholding pep8's white-space conventions.

Included new diffsims.generators.diffraction_generator.AtomicDiffractionGenerator interface along with corresponding back-end code. This is mainly in diffsims.sims.kinematic_simulation which relies on diffsims.utils.discretise_utils to discretise the atomic volume and diffsims.utils.fourier_transform which interfaces various fft utilities. Incomplete tests have been provided for all new modules

robtovey · 2019-11-12T15:32:20Z

Hopefully this is now in a stable state. I think the only things left are to finish the tests and include a demo notebook. Hopefully I'll get around to these by next week.

dnjohnstone · 2019-11-26T09:23:20Z

@robtovey do you need anything from us to move this forward?

robtovey · 2019-11-26T11:00:26Z

@dnjohnstone No, sorry, I'm just being slow to get around to things on my end.

…/diffsims.git into kinematic_simulator-rob

robtovey · 2019-12-03T16:54:17Z

I think this is basically fully covered now. There are three big holes left

the GPU code is not tested by travis
there are several blocks of code designed for anticipating memory errors
numba code does not register for coverage, this makes sense because it compiles the lines differently

What would you like me to do with the remaining bits? I would suggest excluding them from coverage with a comment to say why in each instance?

Otherwise, two remaining tasks are to make a demo and add the copyright etc.

pc494 · 2019-12-06T16:50:12Z

1) the GPU code is not tested by travis
That's fine by me.

2) there are several blocks of code designed for anticipating memory errors
Could you give a bit more detail on this?

3) numba code does not register for coverage, this makes sense because it compiles the lines differently
When you say "does not register" you mean that it claims not to be covered when it is?

What would you like me to do with the remaining bits? I would suggest excluding them from coverage with a comment to say why in each instance?
That sounds like a good plan.

Otherwise, two remaining tasks are to make a demo and add the copyright etc.
Yep, at this point can you just talk about how you expect this code to install/work on machines that both are and aren't equipped with a GPU. It seems that if travis passes you have a fall back for non-GPU work?

robtovey · 2019-12-10T09:45:19Z

1) the GPU code is not tested by travis
That's fine by me.

2) there are several blocks of code designed for anticipating memory errors
Could you give a bit more detail on this?

So the main idea is you want to have a high-resolution discretisation but the full arrays can't all be stored on the GPU at once. In which case the arrays are iteratively partitioned until they do fit onto the GPU. There's a little more to it and it will do the same thing even if you're running on the CPU and running out of RAM but this is the main reason and target for it.

3) numba code does not register for coverage, this makes sense because it compiles the lines differently
When you say "does not register" you mean that it claims not to be covered when it is?

Exactly, I know the functions are called but travis doesn't register any of the lines having been run. This is even true, the actual lines are parsed and compiled by numba but never run by python itself...

What would you like me to do with the remaining bits? I would suggest excluding them from coverage with a comment to say why in each instance?
That sounds like a good plan.

Great, I'll start filling that in.

Otherwise, two remaining tasks are to make a demo and add the copyright etc.
Yep, at this point can you just talk about how you expect this code to install/work on machines that both are and aren't equipped with a GPU. It seems that if travis passes you have a fall back for non-GPU work?

Sure, functionality is the same between the two. Both are (hopefully) trivial to install on anaconda and a bit more effort on pip but still possible. They should look something like:

CPU: conda install numba diffsims
GPU: conda install cudatoolkit numba diffsims
If there is a keyword for using the GPU, on by default, which is silently ignored if cudatoolkit is not installed or if the GPU doesn't activate for some reason.

Excluding GPU code and numba from coverage

dnjohnstone · 2019-12-11T13:41:27Z

@robtovey what's left to do on this? It would be good to get merged up before Christmas!

robtovey · 2019-12-11T16:25:33Z

I think we're pretty close, I now have a demo notebook. I'm not sure quite sure where to put it and I might need a bit of advice on the wording but it does the job. The images are not a great resolution but it all runs in 2 minutes on my CPU so hopefully that tradeoff is appreciated.

Something I thought about, do you want code for computing the volume charge density? I've relatively 'hacked' it in as you will see but should I give it a quick polish too?
Also, I have currently only copied out the data for the first 21 elements of the periodic table... How many would you like?!

Other than that, I think the only remaining thing for me is to test the GPU code as well the we can merge.

Added one line to the coverage

Final coverage ammendment?

robtovey · 2019-12-11T17:16:09Z

Hopefully that is finally back to '100%' coverage...
The GPU passed all my tests locally as well so that should be fine.
For convenience, I have put the notebook here for now. I should probably try to improve the images before it's merged too.

On another note, not sure if it was deliberate or slipped through the cracks but I think there is only 1 line missing from coverage now in the file: here

pc494 · 2019-12-17T18:06:09Z

I am looking to tidy this up and get it merged soon(ish). @robtovey - do you think this is "done" as far as you are concerned?

dnjohnstone

I've made some comments mostly just on naming and naming style - otherwise it basically looks good. I will make a diffsims-demos repository now to put the notebook in.

dnjohnstone · 2019-12-18T09:46:08Z

Diffsims simulate atomic diffraction pattern.ipynb

@@ -0,0 +1,1985 @@
+{


Please can we remove this notebook from here and create a pyxem/diffsims-demos repository for tutorials like this.

dnjohnstone · 2019-12-18T09:49:45Z

diffsims/__init__.py

@@ -22,12 +22,16 @@

 import numpy as np

-from .generators.diffraction_generator import DiffractionGenerator
+from .generators.diffraction_generator import DiffractionGenerator, AtomicDiffractionGenerator


I think we should be able to come up with better names than this - I think the difference is really that we have a structure factor based simulation and an FFT based simulation, we also have (a bit hidden in the code) a real space sum of scattering from many atoms solution. So I think we should either have one DiffractionGenerator with multiple methods or name more based around the simulation approach. I prefer the latter here so maybe we could go with:

SFDiffractionGenerator
FFTDiffractionGenerator
RSDifractionGenerator

Or we could change and have:

StructureFactorSimulator
FFTSimulator
RealSpaceSimulator

What do others think?

I agree with the latter. The main problem I see with trying to merge them into one object is that, as far as I understand it, they just take different inputs (whole crystal vs unit cell) and give different outputs (single diffraction pattern vs complete list of Bragg angles).

I'm not entirely sure what your 'real space' simulator is but I'm not sure about using FFT as part of the naming system. If you add a multi-slice implementation later then I think that will also use FFTs but I guess you'd also want to give it another name? I think these are all fundamentally different models and a choice of backend should be a different parameter/method.

I'm not sure what would be a better name, I guess you will both know much better than me. FirstBorn, Ewald, or something with Kinematic?.. Other than that, I don't have much preference over the general naming format.

diffsims/__init__.py

dnjohnstone · 2019-12-18T09:51:42Z

diffsims/generators/diffraction_generator.py

@@ -201,7 +202,7 @@ def calculate_profile_data(self, structure,
            d_hkl = 1 / g_hkl

            # Bragg condition
-            #theta = asin(wavelength * g_hkl / 2)
+            # theta = asin(wavelength * g_hkl / 2)


If we're not using this, let's remove it.

dnjohnstone · 2019-12-18T09:51:55Z

diffsims/generators/diffraction_generator.py

@@ -228,11 +229,11 @@ def calculate_profile_data(self, structure,
            # Intensity for hkl is modulus square of structure factor.
            i_hkl = (f_hkl * f_hkl.conjugate()).real

-            #two_theta = degrees(2 * theta)
+            # two_theta = degrees(2 * theta)


Likewise, remove if unused.

diffsims/sims/kinematic_simulation.py

dnjohnstone · 2019-12-18T10:06:30Z

diffsims-demos - now here: https://github.com/pyxem/diffsims-demos/

Improved docstrings, copyright notices, and all the scattering parameters from the Peng paper included (98 elements).

robtovey · 2020-01-08T13:31:41Z

I think this is basically ready to merge now. Two outstanding comments, the naming of diffraction generators and moving the jupyter notebook to its new home. I think the second is easier for either of you to do and once you've decided on a naming system I'll be happy to implement?

robtovey · 2020-01-08T13:44:03Z

Sorry, I'm not sure why appveyor is failing. It's complaining about an import of matplotlib which isn't installed but as far as I can tell, this isn't in my code. Have I missed something?

pc494 · 2020-01-09T18:14:54Z

The appveyor thing is unrelated to you @robtovey and I'll (attempt to) deal with it, then get this PR sorted and merge for you.

robtovey · 2020-01-14T09:30:02Z

I've just remembered to talk about installation. The minimum are in the files but there are a couple of packages which will accelerate things, in particular using the GPU and fftw backends.
A full installation would include something like:
conda install cudatoolkit, pyfftw

rob and others added 7 commits November 5, 2019 13:56

Implementation of probes and new dependency on numba

ae80649

added psutil dependency for checking RAM

c074a12

Corrected bug in fourier code

8f710a8

Updated Appveyor dependencies

e0f694e

Update optional user parameters

19cd92f

Merge branch 'master' into kinematic_simulator-rob

1369f48

rob added 3 commits December 3, 2019 15:04

Extended testing for coverage

240660d

Merge branch 'kinematic_simulator-rob' of https://github.com/robtovey…

51595db

…/diffsims.git into kinematic_simulator-rob

Adding test coverage

ea1c515

Coverage updates

bd209f0

Excluding GPU code and numba from coverage

rob added 3 commits December 11, 2019 16:44

Update with demo notebook

bda60b0

There's always one...

40c76aa

Added one line to the coverage

There's always one more...

895990f

Final coverage ammendment?

dnjohnstone reviewed Dec 18, 2019

View reviewed changes

dnjohnstone assigned pc494 and robtovey Dec 18, 2019

dnjohnstone added this to the v0.2.0 milestone Dec 18, 2019

dnjohnstone added the enhancement New feature, request, or improvement label Dec 18, 2019

rob added 2 commits January 8, 2020 13:21

Update to address comments on merge

b218e81

Improved docstrings, copyright notices, and all the scattering parameters from the Peng paper included (98 elements).

Couple of bug fixes

39f972b

Merge branch 'master' into kinematic_simulator-rob

2ea5f77

dnjohnstone merged commit ad3dc68 into pyxem:master Jan 10, 2020

dnjohnstone mentioned this pull request Jan 11, 2020

Remove notebook #44

Merged

dnjohnstone mentioned this pull request Apr 9, 2020

Code performance improvements pyxem/pyxem#585

Closed

robtovey mentioned this pull request Jul 20, 2020

Credits for Rob Tovey #98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFT based kinematic simulator #33

FFT based kinematic simulator #33

robtovey commented Nov 5, 2019 •

edited

Loading

robtovey commented Nov 12, 2019

dnjohnstone commented Nov 26, 2019

robtovey commented Nov 26, 2019

robtovey commented Dec 3, 2019

pc494 commented Dec 6, 2019

robtovey commented Dec 10, 2019 •

edited

Loading

dnjohnstone commented Dec 11, 2019

robtovey commented Dec 11, 2019

robtovey commented Dec 11, 2019

pc494 commented Dec 17, 2019

dnjohnstone left a comment

dnjohnstone Dec 18, 2019

dnjohnstone Dec 18, 2019

robtovey Dec 18, 2019

dnjohnstone Dec 18, 2019

dnjohnstone Dec 18, 2019

dnjohnstone commented Dec 18, 2019

robtovey commented Jan 8, 2020

robtovey commented Jan 8, 2020

pc494 commented Jan 9, 2020

robtovey commented Jan 14, 2020

FFT based kinematic simulator #33

FFT based kinematic simulator #33

Conversation

robtovey commented Nov 5, 2019 • edited Loading

robtovey commented Nov 12, 2019

dnjohnstone commented Nov 26, 2019

robtovey commented Nov 26, 2019

robtovey commented Dec 3, 2019

pc494 commented Dec 6, 2019

robtovey commented Dec 10, 2019 • edited Loading

dnjohnstone commented Dec 11, 2019

robtovey commented Dec 11, 2019

robtovey commented Dec 11, 2019

pc494 commented Dec 17, 2019

dnjohnstone left a comment

Choose a reason for hiding this comment

dnjohnstone Dec 18, 2019

Choose a reason for hiding this comment

dnjohnstone Dec 18, 2019

Choose a reason for hiding this comment

robtovey Dec 18, 2019

Choose a reason for hiding this comment

dnjohnstone Dec 18, 2019

Choose a reason for hiding this comment

dnjohnstone Dec 18, 2019

Choose a reason for hiding this comment

dnjohnstone commented Dec 18, 2019

robtovey commented Jan 8, 2020

robtovey commented Jan 8, 2020

pc494 commented Jan 9, 2020

robtovey commented Jan 14, 2020

robtovey commented Nov 5, 2019 •

edited

Loading

robtovey commented Dec 10, 2019 •

edited

Loading