-
Notifications
You must be signed in to change notification settings - Fork 52
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
Inverse pole density distribution 2 #362
Inverse pole density distribution 2 #362
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.
This is a very nice PR with robust testing and neat reusing of one function to provide the inverse pole density function calculations to three classes Vector3d
, StereographicPlot
and InverseStereographicPlot
.
Should the IPDF be used in a user guide tutorial (notebook)? That can be left for another PR, of course, but should perhaps be done before the next release.
I've made several suggestions for changes, mostly in the docstrings and slight changes to code. Otherwise, I think this is good to go.
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]>
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]>
I've resolved all but one insignificant comment on comments... Feel free to ping me when you wan't a second review! |
Thanks, I had missed a couple abbreviations in the inline comments- this should be good for a second look! |
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.
Found a couple of things that should be changed, sorry for missing these in the first review.
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]>
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 looks very good to me! Will let you merge once all checks pass and you're happy.
Thanks for the review @hakonanes, I am happy with this. I will just wait to see if @pc494 has any comments on #351, as you commented, before merging. |
Apologies for the delay. I think |
Description of the change
This is the new PR for #355 after that PR was pre-maturely merged.
Previous PR description below
Following on from #340, it is often useful to view the pole density function (PDF) in the fundamental sector of the crystal symmetry, ie. the inverse pole density function (IPDF).
To implement this, the
StereographicPlot.pole_density_function()
has been updated to work with the subclassInverseStereographicPlot
. When calculating PDF, the axes are checked as to whether they have a_symmetry
attribute, ie.IPF
, and if so the calculatedPDF
is folded back into the fundamental sector.At the moment there are some issues with the implementation, regarding aliasing (during folding procedure) and the axes limits (which I may be due to
QuadMesh
, see plot size below), see the examples below. Grateful for any suggestions, feedback, or help regarding this!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
.