-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add CTF file reader and interactive crystal map plot example #451
Conversation
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.
Thank you for this reader, @IMBalENce! It will make orix accessible to many more people.
With this PR I could read the only AZtec HKL CTF file I have on my computer (I got it from you last year). The can be simplified a lot, since many parts adopted from the .ang reader are unnecessary, I think. See comments.
The example is very nice, and a great addition to the docs! The next thing would be to navigate with the arrow keys... Another PR for another time.
orix/io/__init__.py
Outdated
@@ -81,13 +81,19 @@ def loadctf(file_string: str) -> Rotation: | |||
file_string | |||
Path to the ``.ctf`` file. This file is assumed to list the | |||
Euler angles in the Bunge convention in the columns 5, 6, and 7. | |||
The starting row for the data that contains Euler angles is relevant |
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.
With the new ctf reader you're adding, I suggest to leave this function as is and instead deprecate it in favor of the new one. We can then remove it in v0.13. What do you think?
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.
Sounds good to me. I was also planning to add a ctf file writer at some stage.
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.
Great. Please tell me if you need some hints on deprecation.
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.
Thank you so much for your time on reviewing my changes @hakonanes. A lot of very helpful suggestions! I just realize some of the test did not pass. I'll work on the test code. |
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.
Good work! Here are a few more comments.
I've found five different versions of the CTF format among the files collaborators have kindly shared with me over the years. Oxford Instruments, Bruker, NanoMegas ASTAR, EMsoft and MTEX can all write to this format. Your reader could read all of them (great!), but there are some slight differences and pitfalls which we need to address to support all of them. Are you OK with me contributing to your branch with a few commits to account for these differences?
Btw, I find it easiest if the reviewer resolves a conversation.
orix/io/__init__.py
Outdated
@@ -81,13 +81,19 @@ def loadctf(file_string: str) -> Rotation: | |||
file_string | |||
Path to the ``.ctf`` file. This file is assumed to list the | |||
Euler angles in the Bunge convention in the columns 5, 6, and 7. | |||
The starting row for the data that contains Euler angles is relevant |
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.
Great. Please tell me if you need some hints on deprecation.
orix/io/plugins/ctf.py
Outdated
0: [ | ||
"phase_id", | ||
"x", | ||
"y", | ||
"bands", | ||
"error", | ||
"euler1", | ||
"euler2", | ||
"euler3", | ||
"MAD", # Mean angular deviation | ||
"BC", # Band contrast | ||
"BS", # Band Slope | ||
], | ||
}, |
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 columns are the same in the Bruker CTF files I have access to.
@IMBalENce, do you think we could finish this work? If you don't have time, no worries, I can look at finishing myself. |
@hakonanes Sorry for taking so long. It was a bit hectic lately. I'll do my best to work this out in two weeks. |
No problem! I've been off development myself lately. |
@hakonanes I tried my best to address each of your comments. I think most should be ok now. I also found some issue in running pytest and can get stuck in Additionally, I'm still working on the test coverage on this ctf reader. I'm hoping to complete it in a day or two. |
By stuck, do you mean that pytest hangs? Or that you get an error? It would be great if you could open an issue. We'd be much closer to a solution, then! |
Any movement on this lately? I can take a shot at debugging the failing test if that is helpful. Is that that only thing that is blocking this from merging? |
@IMBalENce The failing test is related to the fact that there used to be a warning for trying to read a .ctf file. |
Signed-off-by: IMBalENce <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
Co-authored-by: Håkon Wiik Ånes <[email protected]>
@CSSFrancis Sorry for the delay in moving this forward again. Thank you for the tips. I'm testing it now. Will get it back soon. |
I'm ready to help and review, btw. |
@IMBalENce, I'm looking over the PR now. I have a few suggestions for changes. I'll make a PR to your branch if that's OK. |
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Thank you @hakonanes. Have just merged your changes. |
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
This should now be done. Could you have a brief look at my latest changes, @IMBalENce? These are the changes since you merged my PR to your branch: 2b3b7f0...a68d37e. |
Signed-off-by: Håkon Wiik Ånes <[email protected]>
Signed-off-by: Håkon Wiik Ånes <[email protected]>
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 CTF reader should now be able to read files created by Oxford Instruments, Bruker, ASTAR, EMsoft, and MTEX.
Note, I've contributed code to this PR myself, so a second look my another maintainer would be appreciated.
Thank you, @hakonanes. It looks great! |
Fantastic, thanks for having a look, @IMBalENce! |
Description of the change
Following up #412, added a file reader for Oxford AZtec HKL .ctf file.
Added an example jupyter notebook for interactive plotting - clicking on an IPF map to retrieve phase name and Euler angles.
Progress of the PR
Minimal example of the bug fix or new feature
For reviewers
__init__.py
.section in
CHANGELOG.rst
.__credits__
inorix/__init__.py
and in.zenodo.json
.