Skip to content
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

Allow not passing simulation indices property array to crystal map merging #335

Merged
merged 5 commits into from
Apr 15, 2021

Conversation

hakonanes
Copy link
Member

Description of the change

The fact that we now have to pass simulation_indices_prop to merge_crystal_maps() in order to get the simulation indices in the merged map, instead of getting these by default, is breaking the API. However, I will ignore that fact and include this in the forthcoming v0.3.3 patch release.

Progress of the PR

Minimal example of the bug fix or new feature

The following didn't work previously, but now works

import kikuchipy as kp
import numpy as np
from orix import crystal_map, quaternion

nav_shape = (3, 3)
ny, nx = nav_shape
nav_size = np.prod(nav_shape)
r = quaternion.Rotation.from_euler(np.ones((nav_size, 3)))
x = np.tile(np.arange(ny), nx)
y = np.repeat(np.arange(nx), ny)

# Simulation indices
n_sim_indices = 10
sim_indices1 = np.random.randint(
    low=0, high=1000, size=n_sim_indices * nav_size
).reshape((nav_size, n_sim_indices))
sim_indices2 = np.random.randint(
    low=0, high=1000, size=n_sim_indices * nav_size
).reshape((nav_size, n_sim_indices))

# Scores
scores1 = np.ones(nav_size)
scores1[0] = 3
scores2 = 2 * np.ones(nav_size)

xmap1 = crystal_map.CrystalMap(
    rotations=r,
    phase_id=np.ones(nav_size) * 0,
    x=x,
    y=y,
    prop={"simulation_indices": sim_indices1, "scores": scores1},
)
xmap2 = crystal_map.CrystalMap(
    rotations=r,
    phase_id=np.ones(nav_size),
    x=x,
    y=y,
    prop={"simulation_indices": sim_indices2, "scores": scores2},
)
xmap_merged = kp.indexing.merge_crystal_maps(
    crystal_maps=[xmap1, xmap2],
#    simulation_indices_prop="simulation_indices",
)

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the
    unreleased section in doc/changelog.rst.

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes hakonanes added bug Something isn't working 👈 backport labels Apr 15, 2021
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Copy link
Collaborator

@onatlandsmyr onatlandsmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this will fix the issues at hand, looks good. The docstring for merge_crystal_maps, "It is required that there are ... ", should be updated to reflect that it's no longer required with simulation indices.

@hakonanes
Copy link
Member Author

Good catch! I will change the wording to

It is required that all maps have the same number of rotations, scores, and simulation indices if applicable, per point.

@hakonanes
Copy link
Member Author

Rather:

It is required that all maps have the same number of rotations and scores (and simulation indices if applicable) per point.

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes
Copy link
Member Author

Thanks @onatlandsmyr! Will merge after checks pass.

@hakonanes hakonanes merged commit a1b5ef4 into pyxem:master Apr 15, 2021
@hakonanes hakonanes deleted the fix-323 branch April 15, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot merge CrystalMap's with fewer refined orientations than kept dictionary indices
2 participants