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

Add Sampling #460

Merged
merged 11 commits into from
Mar 29, 2024
Merged

Add Sampling #460

merged 11 commits into from
Mar 29, 2024

Conversation

CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented Oct 26, 2023

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

from orix.symmetry import C2
from orix.sampling import get_sample_zone_axis
sampling = get_sample_zone_axis(2.0, point_group=C2)

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 CHANGELOG.rst.
  • Contributor(s) are listed correctly in __credits__ in orix/__init__.py and in
    .zenodo.json.

@CSSFrancis CSSFrancis force-pushed the add_sampling branch 2 times, most recently from efd13f2 to afbd20b Compare October 27, 2023 16:35
@CSSFrancis
Copy link
Member Author

@hakonanes Sorry for making you rerun the workflows a bunch....

@CSSFrancis
Copy link
Member Author

@hakonanes Any now I didn't have pre-commit set up correctly.... We should (finally be good)

@CSSFrancis
Copy link
Member Author

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?

@hakonanes hakonanes mentioned this pull request Nov 2, 2023
4 tasks
@hakonanes
Copy link
Member

Hmm I'm not sure why my local pre commit isn't agreeing with orix

The two code style errors are related to sorting of imports. Do you have isort installed? It's among the dev dependencies.

@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?

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.

@hakonanes
Copy link
Member

And, please request a review from me when you're ready!

@hakonanes hakonanes added the enhancement New feature or request label Nov 3, 2023
@hakonanes hakonanes added this to the v0.12.0 milestone Nov 3, 2023
@hakonanes
Copy link
Member

Also, if you rebase on develop, the docs should be passing.

@CSSFrancis
Copy link
Member Author

@hakonanes I've updated the head comment with more information.

@CSSFrancis
Copy link
Member Author

pre-commit.ci autofix

@CSSFrancis CSSFrancis mentioned this pull request Mar 27, 2024
16 tasks
@pc494
Copy link
Member

pc494 commented Mar 27, 2024

Should be able to review this for you tomorrow @CSSFrancis.

@CSSFrancis
Copy link
Member Author

@pc494 Sounds good! I'll see if I can determine why the test isn't passing.

@pc494
Copy link
Member

pc494 commented Mar 28, 2024

@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.

@CSSFrancis
Copy link
Member Author

@pc494 yea I can't tell if that test is flakey or there is some scipy bug related to certain hardware configurations.

Copy link
Member

@pc494 pc494 left a 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.

@pc494 pc494 self-requested a review March 28, 2024 20:08
Copy link
Member

@pc494 pc494 left a 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.

@pc494 pc494 self-requested a review March 29, 2024 20:38
Copy link
Member

@pc494 pc494 left a 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.

@pc494 pc494 merged commit e658e7b into pyxem:develop Mar 29, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants