Consolidate generator functions with `diffsims`

[harripj]

There is a bug in sample_S2_uv_mesh due to inverted ranges for the azimuthal and polar angles. This can be fixed in a PR with some added tests, but as this code is essentially duplicated from diffsims I think it would be good to discuss where this code should live first, to improve code reuse and reduce bugs.

Both orix and diffsims have overlapping generator functions and I think it would be wise to consolidate these into just one package. Given the tight relationship between both packages I don't think this would cause too many problems. I know in my experience I often have to check in which package these generator functions live, so this would help in this case too. I see that diffsims imports from orix and not vice-versa and, to avoid circular imports, perhaps this is why some of these functions were duplicated into orix in the first place?

I think it would be a good idea to bring the sphere_mesh_generators module into orix, for example get_uv_sphere_mesh_vertices and sample_S2_cube_mesh already exist in orix's S2_sampling module, as discussed. I think the rotation_list_generators may also be a good idea to move over, but not so sure on this.

All of this said, I do not use diffsims too much so forgive me if I this is a reach- it would be great to get some thoughts on this from the diffsims developers!

[hakonanes]

There is a bug in sample_S2_uv_mesh due to inverted ranges for the azimuthal and polar angles.

Where does it show itself? Could you add an example?

In short, we should move S2 sampling to orix, which I believe there is concensus on. See my https://github.com/pyxem/orix/pull/235#issuecomment-951118478 and the following comment by @din14970. He asks us to maintain the diffsims API for now. If we port all functionality, we should deprecate it in diffsims for at least one minor release.

This makes me think of a workflow @emichr recently wrote to restrict the rotation list of Euler angles in diffsims to a certain misorientation angle about a certain zone axis, using orix. It would be good to add these use cases to a "Vectors" section in the user guide.

[harripj]

Where does it show itself? Could you add an example?

This is the example using the current code:

resolution = 10
steps_azimuth = int(np.ceil(360 / resolution))
steps_polar = int(np.ceil(180 / resolution)) + 1
azimuth = np.linspace(0, np.pi, num=steps_azimuth, endpoint=True)
polar = np.linspace(0, 2 * np.pi, num=steps_polar, endpoint=False)
azimuth_prod, polar_prod = np.meshgrid(azimuth, polar)
azimuth_prod = azimuth_prod.ravel()
polar_prod = polar_prod.ravel()
v = Vector3d.from_polar(azimuth=azimuth_prod, polar=polar_prod).unit

fig, ax = plt.subplots(subplot_kw=dict(projection='3d'))
ax.scatter(*v.data.T)

For a resolution of 10 there should be 19 elevation levels, which is not the case:

The bug is that azimuth should range from [0..2*pi) and polar from [0..pi], and in the current implementation they are inverted. Using the updated code:

steps_azimuth = int(np.ceil(360 / resolution))
steps_polar = int(np.ceil(180 / resolution)) + 1
azimuth = np.linspace(0, 2 * np.pi, num=steps_azimuth, endpoint=False)  # updated
polar = np.linspace(0, np.pi, num=steps_polar, endpoint=True)  # updated
azimuth_prod, polar_prod = np.meshgrid(azimuth, polar)
azimuth_prod = azimuth_prod.ravel()
polar_prod = polar_prod.ravel()
v_new = Vector3d.from_polar(azimuth=azimuth_prod, polar=polar_prod).unit

fig, ax = plt.subplots(subplot_kw=dict(projection='3d'))
ax.scatter(*v_new.data.T)

This updated code is more in agreement with diffsims:

from diffsims.generators import sphere_mesh_generators

v = sphere_mesh_generators.get_uv_sphere_mesh_vertices(10)
fig, ax = plt.subplots(subplot_kw=dict(projection='3d'))
ax.scatter(*v.T)

And in fact the implementation in diffsims makes sure there is only one point at the north and south pole, whereas in the orix implementation, currently there are many.

In short, we should move S2 sampling to orix, which I believe there is concensus on. See my https://github.com/pyxem/orix/pull/235#issuecomment-951118478 and the following comment by @din14970. He asks us to maintain the diffsims API for now. If we port all functionality, we should deprecate it in diffsims for at least one minor release.

This would be an option. As @din14970 mentioned, the functions in diffsims could be maintained as wrappers to the code when ported to orix.

[hakonanes]

I ported @din14970's code to orix, so this error was introduced by me. Thank you for noticing! Feel free to make a PR fix.

[din14970]

I think everything related to vectors, crystallography, rotations and orientations should live in orix. With the recent work you guys have done for determining fundamental zones for different point groups, it makes even more sense to integrate "point list" generators into orix instead of maintaining an inferior parallel flow in diffsims. Indeed the 1 for 1 identical functionality should become depricated, though I would like to build utility functions in diffsims that allows one to generate a library of patterns for a specific crystal, without the user having to import a bunch of stuff from another package.

[hakonanes]

though I would like to build utility functions in diffsims that allows one to generate a library of patterns for a specific crystal, without the user having to import a bunch of stuff from another package

Sounds reasonable. It would be good to document in diffsims (docstrings, or potential user guides) that if a user desires, they have freedom to manipulate the rotation list with orix based on "crystallographic" conditions.

[hakonanes]

Rephrased the issue title to reflect that the bug is fixed. We can continue discussion of porting diffsims functionality to orix here.

[harripj]

@din14970 is working on this in #334.

[harripj]

This was closed by #334.