-
Notifications
You must be signed in to change notification settings - Fork 53
Add Sampling #460
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 Sampling #460
Conversation
efd13f2
to
afbd20b
Compare
@hakonanes Sorry for making you rerun the workflows a bunch.... |
afbd20b
to
7fd812b
Compare
@hakonanes Any now I didn't have pre-commit set up correctly.... We should (finally be good) |
Hmm I'm not sure why my local pre commit isn't agreeing with orix. 😅 probably some version mismatch or it not running properly for some reason. @hakonanes any problems with my adding the pre-commit ci to Orix? That way it will run isort and black on the code either automatically or when you ask it to? |
The two code style errors are related to sorting of imports. Do you have
We could add a way to run pre-commit automatically if asked. I'm open to that. However, I think there is a point where there is too much automation in the development process for us to maintain, instead of focusing on functionality. I consider pushes by bots too much. At least for this project at this point in time. Anyways, I wouldn't worry too much about the code style now. Could you explain a bit more in the top comment what this new functionality does and what its intended use is? Any details about the implementation you can provide would be much appreciated. |
And, please request a review from me when you're ready! |
Also, if you rebase on |
7fd812b
to
f0dcb55
Compare
@hakonanes I've updated the head comment with more information. |
c4d9ccc
to
75974fe
Compare
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Should be able to review this for you tomorrow @CSSFrancis. |
@pc494 Sounds good! I'll see if I can determine why the test isn't passing. |
I think that test might be the flaky one. |
@pc494 yea I can't tell if that test is flakey or there is some scipy bug related to certain hardware configurations. |
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.
Thanks @CSSFrancis, this looks good to me.
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.
Sorry, I spoke too soon, this still needs a Changelog entry + imports as-per the PR template.
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.
Sorry, had missed that you already had the imports, merging.
Description of the change
Add in Sampling using reduced symmetry sampling.
Sampling Methods Added:
The first sampling functionality mirrors the S_2 sampling tutorial, but is more explicit such that given some point group the rotations which rotate some vector [0,0,1] to each sampling vector is returned. This is used when fast template matching in
pyxem
.The second sampling method is just a simple method for finding zone axes in simple systems. I think there are much better ways of doing this and I'd like to improve this method from the old
diffsims
one (and I think kikuchipy already does some of that).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
.