orix
orix copied to clipboard
Change rounding from 12 to 10 decimals when finding unique vectors and rotations
Description of the change
Changing rounding from 12 to ~11~ 10 decimals before finding unique vectors and rotations fixes a case where too many plane normals were returned from Miller.symmetrise(unique=True)
. This fixes #404 reported @Martin-Rudolph. To be consistent, I changed all rounding in orix where 12 decimals was used.
This PR is aimed at main
, so that we can release this fix immediately as a v0.10.2 patch. I set the release date in the changelog to ~the coming Friday 21 October~ today.
I've also made minor changes to type hints and formatting to silence some warnings raised by my code editor.
Progress of the PR
- [n/a] Docstrings for all functions
- [x] Unit tests with pytest for all lines
- [x] Clean code style by running black via pre-commit
Minimal example of the bug fix or new feature
The issue reported by @Martin-Rudolph
>>> from diffpy.structure import Lattice, Structure
>>> from orix.crystal_map import Phase
>>> from orix.vector import Miller
>>> lat_hex = Lattice(3.073, 3.073, 10.053, 90, 90, 120)
>>> phase = Phase(point_group="6/mmm", structure=Structure(lattice=lat_hex))
>>> h = Miller(UVTW=[1, -1, 0, 0], phase=phase)
>>> h.symmetrise(unique=True) # Not all unique
Miller (10,), point group 6/mmm, UVTW
[[ 1. -1. 0. 0.]
[ 0. 1. -1. 0.]
[-1. 0. 1. 0.]
[-1. 1. -0. 0.]
[ 0. -1. 1. 0.]
[ 1. -0. -1. 0.]
[ 1. 0. -1. 0.]
[-1. 1. 0. 0.]
[-1. -0. 1. 0.]
[ 1. -1. -0. 0.]]
With this fix, the last call returns
>>> h.symmetrise(unique=True) # All unique
Miller (6,), point group 6/mmm, UVTW
[[ 1. -1. 0. 0.]
[ 0. 1. -1. 0.]
[-1. -0. 1. 0.]
[-1. 1. -0. 0.]
[ 0. -1. 1. 0.]
[ 1. 0. -1. 0.]]
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__
inorix/__init__.py
and in.zenodo.json
.
The failing test on Windows Python 3.9 is reported in #403 and fixed in #402.
@hakonanes thanks for doing this, I seem to remember we foresaw this problem at the time. I wonder whether you think there is any utility in rounding to 10dp, or adding a decimal_places kwarg to the affected functions, eg. unique?
I wonder whether you think there is any utility in rounding to 10dp
To be on the safe side? I'm fine with that as long as all tests pass (which they do on my machine)! I'll do this.
or adding a decimal_places kwarg to the affected functions, eg. unique?
Yes, this seems like the simplest solution. This way the precision can be set based on an application's requirement. It wouldn't have fixed the issue with Miller.symmetrise(unique=True)
though, unless the internal call to Miller.unique()
set the precision to 11 or lower.
This should be done in a separate PR to develop
, to be released in the next minor v0.11.0, right?
I wonder whether you think there is any utility in rounding to 10dp
To be on the safe side? I'm fine with that as long as all tests pass (which they do on my machine)! I'll do this.
or adding a decimal_places kwarg to the affected functions, eg. unique?
Yes, this seems like the simplest solution. This way the precision can be set based on an application's requirement. It wouldn't have fixed the issue with
Miller.symmetrise(unique=True)
though, unless the internal call toMiller.unique()
set the precision to 11 or lower.This should be done in a separate PR to
develop
, to be released in the next minor v0.11.0, right?
still merging this, because @hakonanes is correct that new functionality needs to be saved for 0.11.0
Thank you both for quickly responding to this fix @harripj and @pc494. I've changed from 11 to 10 decimals.
Ideally, I would wait for @Martin-Rudolph to confirm that this branch fixes his issue so that Miller.symmetrise(unique=True)
returns what he expects.
will hold fire on the merge then :)
This PR can be merged once checks pass (the single fail is OK)! I plan to release 0.10.2 once it is merged.
Thanks a lot Phillip! Just published the automatically created release, and now orix 0.10.2 is out on PyPI. Will follow up with a post-release PR to develop
and a conda-forge release once their bot makes a PR.