-
Notifications
You must be signed in to change notification settings - Fork 84
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
Refactor Orientation Mapping(Try 3) #1076
base: main
Are you sure you want to change the base?
Conversation
dc562b0
to
746072a
Compare
@viljarjf I tried to start testing to make sure that the This failing test is: |
Template matching suffers from misindexation quite often, which is probably what is happening here. I tried replacing the random orientations of the grains with some easier zone axes, and it works much better. |
Please do! |
@pc494 Any chance you can look this over? We can merge it once the new simualtions for diffsims are relased and maybe think about what it might take to make a rc. |
@viljarjf I've been playing around with this a little bit more. We can pretty easily do something like this: Which I think seems useful but this problem of misorientations is something that is bothering me slightly. Maybe the problem is that even slightly more complicated crystal strucutures like Si start to cause issues when doing any orientation mapping. It might be worth while to be able to plot the 3-4 best fit but non-clustered fits otherwise we are just plotting the same cluster over and over again. |
Just following up on this a little bit more: from pyxem.data import si_phase, si_grains, si_rotations_line
from diffsims.generators.simulation_generator import SimulationGenerator
from orix.sampling import get_sample_reduced_fundamental
import hyperspy.api as hs
#simulated_si = si_grains(recip_pixels=256)
simulated_si = si_rotations_line()
simulated_si = hs.stack([simulated_si,simulated_si])
# %%
# First we center the diffraction patterns and get a polar signal
# Increasing the number of npt_azim with give better polar sampling
# but will take longer to compute the orientation map
# The mean=True argument will return the mean pixel value in each bin rather than the sum
# this makes the high k values more visible
simulated_si.calibration.center = None
polar_si = simulated_si.get_azimuthal_integral2d(
npt=100, npt_azim=360, inplace=False, mean=False
)
polar_si.plot()
# %%
# Now we can get make a simulation. In this case we want to set a minimum_intensity which removes the low intensity reflections.
# we also sample the S2 space using the :func`orix.sampling.get_sample_reduced_fundamental`
phase = si_phase()
generator = SimulationGenerator(200, minimum_intensity=0.015)
rotations = get_sample_reduced_fundamental(resolution=1, point_group=phase.point_group)
sim = generator.calculate_diffraction2d(
phase, rotation=rotations, max_excitation_error=0.05, reciprocal_radius=1.5, with_direct_beam=False
)
orientation_map = polar_si.get_orientation(sim,frac_keep=.2, n_best=2, normalize_templates=True)
navigator = orientation_map.to_navigator()
# %%
# sphinx_gallery_thumbnail_number = 3
%matplotlib ipympl
ms = orientation_map.to_ipf_markers()
simulated_si.plot()
simulated_si.add_marker(ms)
simulated_si.add_marker(
orientation_map.to_single_phase_markers(n_best=1, include_intensity=True, intesity_scale=20, lazy_output=True)) This code basically looks at all possible rotations in the reduced s2 space and tries to perform the best template match. The plotting will show the inverse pole figure and the best match found for each rotation. Interestingly there are kind of "dead spots" when you plot the orientations. This might be somewhat related to the number of diffraction vectors at each position which might not be fully being considered by taking the row norm before template matching. @din14970 Do you have any ideas? from orix.vector import Vector3d
orients = orientation_map.to_single_phase_orientations()
vectors = (orients*Vector3d.zvector())
vectors = vectors.in_fundamental_sector(orientation_map.simulation.phases.point_group)
vectors.scatter() |
f0f0e16
to
c52505f
Compare
I think the inset plot with the IPF of the TM result is a very useful addition! Looking forward to exploring some data with it. I am looking at the conventions and such, and fixing up the polar markers. I believe the |
This seems like a non-trivial issue with template matching based orientation mapping. It does seem like maybe the correlation score is not the correct metric for mesuring simularity but that problem seems complicated, and is even more complicated when considering dynamical diffraction. Maybe the best thing is to identify regions with high uncertainty given a crystal strucuture and then report that as well.
Thanks! I'm actually pretty proud of that. It's the culmination of about a year and a halfs work so I'm glad that it worked out :).
Don't worry about it too much. I completely understand! I'm going through something similar. Any time you have is greatly appricated! |
@CSSFrancis and @viljarjf These types of things I wrestled with extensively while I was implementing template matching and was trying it on my own data, and I also wrote about those details in the paper. The main point is that the simple correlation score is fine if you take care to carefully prepare images and templates before indexing. This is also done in commercial software like ASTAR, but it happens automatically with sensible defaults, so most users are unaware and don't think about it. A different metric will not provide you with a magic pill for many reasons which are detailed in the old papers by Edgar Rauch (cited in the paper). Other metrics will however ensure you get much degraded performance, which is important for a brute force algorithm like template matching. It is much more efficient to preprocess both images and potentially templates to guide the correlation score to the right answer. Some things to consider:
Basically, the reasoning mistake that is made in template matching is to equate intensity of a diffraction spot with its relevance to determining orientation. What one really would like to maximize is the number of spots that can be matched between image and template, and minimize the number of spots in the template that do not appear in the image. A number of preprocessing strategies can be employed to guide the process in this direction:
Typically, this should give decent converged results, and it is how I was able to reproduce indexation results from the commercial software. One should keep in mind however that precision is always a function of orientation, and some orientations are more difficult to accurately index. Hope this provides some ideas to interpret what you are seeing in your results. |
@din14970 That was a fantastic discription and something that we should probably add to the documenation! I gathered some of these things from following the notebook you added but I didn't fully understand why certain choices were made.
That's a good trick as well and maybe deserves a seperate example describing of the effects of recentering the data. I wonder if you could do something like disk template matching first (don't find the peak positions) and then caluclate the correlation. That has the benefit of removing noise and setting non-disklike features to negative values.
Thank you! They are mostly just your results at the moment :) I am just trying to reproduce the results in the orientation mapping paper. |
Okay we are starting to get somewhere with this :) I might try to remake the g-phase tutorial as well. We should be able to create a polar mask and then pass the mask alongside the signal for indexing. That should allow us to:
Does that sound reasonable? @viljarjf can you review some of the new parts if you have a chance? I'm going to start writing some tests for this and cleaning up the documentation. |
# Use the quaternion data from rotations to support 2D rotations, | ||
# i.e. unique rotations for each navigation position | ||
rotations = hs.signals.Signal2D(self.simulation.rotations.data) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where this is useful? Currently this isn't really supported by the simulations. (All of the rotations are used so passing as a hyperspy signal doesn't do much). It just casts this to a numpy array and then passes that numpy array to all of the signals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is left over for some stuff I did in my Master's, where each navigation position had a unique set of simulations. By converting it into a signal, they correctly distributed when using signal.map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viljarjf Hmmm That's interesting. I can think about how to best include this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to support it for now, we can add it without breaking the API later if we want
phases = PhaseList(list(self.simulation.phases)) | ||
if phase_index.ndim == 3: | ||
phase_index = phase_index[..., 0] | ||
phase_index = phase_index.flatten() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pc494 or @hakonanes We only pass the phase of the top matching pattern but I wonder what changes in orix would be necessary to have multiple phase indexs similar to how multiple rotations are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I allowed multiple rotations because EMsoft, and eventually kikuchipy, returns those. I seldom use them myself, other than to generate a so-called orientation similarity map (see the Notes).
Is it enough to return multiple crystal maps if a user wants to have a look at the lesser matching solutions? My fear is that this would be a little-used feature that adds a high degree of complexity to the crystal map implementation...
Looking at workflows more broadly, the end-goal is a crystal map with the best possible solution for each point. My feeling is that evaluating the lesser matches should be done by something else than the end-crystal map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hakonanes that sounds reasonable. I don't want to add too much complexity when you are right. Multiple crystal maps would solve the problem.
I guess we need a couple more tools for refining the fit, although I'm not entirely sure what those are :)
pyxem/signals/indexation_results.py
Outdated
) | ||
yield markers | ||
|
||
if annotate: | ||
texts = vectors.map( | ||
vectors_to_text, | ||
inplace=False, | ||
lazy_output=lazy_output, | ||
ragged=True, | ||
output_dtype=object, | ||
output_signal_size=(), | ||
) | ||
coords.map(lambda x: x + annotation_shift, inplace=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viljarjf Is there a good argument for using yield here?
This probably has to be refactored eventually once I get around to making a hs.compute()
. Ideally we would go:
Create a big list of signals --> mass compute with hs.compute which merges the task graphs --> Make a list of markers from the computation.
For the lazy markers, we need to adjust how hyperspy handles plotting of markers.
Currently each marker is computed seperately and the signal is computed seperately as well where we should just merge the task graphs into one. That would require a context manager --> to_compute
attribute for hyperexplorer and then that gets checked/ run on each action.
@ericpre I think we talked about this when first creating the lazy markers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used yield to hopefully reduce memory usage, but using lazy markers would mitigate this
I'm going to clean this up a little bit today and then maybe we can think about merging this? It's starting to grow to an "unreviewable" size. It would be nice to have this in so we can think about making a |
pyxem/data/simulation_fe.py
Outdated
rotations1 = Rotation.random(num_grains) | ||
rotations2 = Rotation.random(num_grains) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if Orix supports setting the seed. In any case, these should really also be consistent between calls with the same seed, especially as correlation score distribution from templatematching are very orientation dependent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the distribution of the rotations does not matter, then we could simply sample e.g. Euler angles from numpy to enforce consistent RNG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, orix doesn't support a seed. Thats a good suggestion but also something that would be useful to add to orix in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just leave this as is and then add that once orix supports it.
pyxem/signals/indexation_results.py
Outdated
(original_offset - ((maxes + mins) / 2)) / (maxes - mins) * 0.2 | ||
) | ||
original_offset = original_offset + 0.85 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple different magic numbers in this function. I have only tested on cubic data, have you tried different (larger) point groups to see if the reduced zone IPF fits correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work as expected but I haven't been super thourough about testing this.
9a7ee01
to
3b25305
Compare
Okay any last comments before merging this @viljarjf? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested sign-posting to https://doi.org/10.1016/j.ultramic.2022.113517 because it has useful explanation on the approach used here. For sure, there will be other useful reference to add, but these can be added later.
@ericpre Sounds like a great idea! Let me do a little bit better of a job using |
I think this has been partially reviewed a couple of times along the way so I am going to merge it at the end of the day today unless there are any objections? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recent changes look good to me. I have not gotten around to trying them out yet, I'll try tomorrow but it might be merged by then
5d18e7d
to
a7c8864
Compare
This merges a couple of different PRs:
#1018
#1061
And adds in som examples. Most of the code additions are related to the examples.