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

Renaming EBSD.match_patterns() to EBSD.dictionary_indexing() #355

Closed
Tracked by #358
hakonanes opened this issue May 7, 2021 · 4 comments · Fixed by #376
Closed
Tracked by #358

Renaming EBSD.match_patterns() to EBSD.dictionary_indexing() #355

hakonanes opened this issue May 7, 2021 · 4 comments · Fixed by #376
Milestone

Comments

@hakonanes
Copy link
Member

hakonanes commented May 7, 2021

We discussed this when introducing pattern matching in kikuchipy in November (#234 (comment)). For the record, I was the one to suggest pattern matching instead of dictionary indexing. But I'm now reconsidering this choice, and although our implementation of dictionary indexing is more rudimentary than in EMsoft, I think it is OK to call our implementation dictionary indexing as well.

Some of the benefits of calling the method EBSD.dictionary_indexing():

  • Inform the user of which pattern matching approach is performed
  • Appropriate crediting of the dictionary indexing approach
  • We can add other pattern matching approaches in other EBSD methods without having naming issues. This is better than having other approaches branch out in match_patterns() by specifying a method=dictionary-indexing etc.

If people agree, we should deprecate EBSD.match_patterns() in v0.4 and remove it in v0.5.

@hakonanes hakonanes added this to the v0.4.0 milestone May 7, 2021
@pc494
Copy link
Member

pc494 commented May 7, 2021

Yeah I would call this dictionary_indexing, I think template_matching (in pyxem) may well go the same way (eventually)

@hakonanes
Copy link
Member Author

Thanks for giving your thoughts @pc494!

@hakonanes hakonanes mentioned this issue May 26, 2021
5 tasks
@hakonanes
Copy link
Member Author

@friedkitteh and I discussed renaming of MasterPattern.get_patterns() to MasterPattern.get_dictionary().

get_dictionary() might be a more suitable name, since a fixed projection center is required. However, I frequently use get_patterns() to project only a few patterns onto the detector, which can't be considered a dictionary. Therefore, I suggest we keep the name get_patterns(), but allow passing an EBSDDetector with as many projection centers as rotations. We would then need to re-calculate the direction cosines per projected pattern, but that shouldn't be too difficult to set up.

What do you think of this, @friedkitteh?

@friedkitteh
Copy link
Collaborator

I think that it is a good change that will for sure come in handy. During the set up I think we can sneak in a some QoL changes like this and possibly some small performance upgrades as a lot of the functions used in the simulation are very general and perhaps using private, more efficient methods would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants