Change rounding from 12 to 10 decimals when finding unique vectors and rotations

hakonanes commented 1 year ago

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 Tuesday, 2022-10-25.

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__ in orix/__init__.py and in .zenodo.json.

hakonanes commented 1 year ago

The failing test on Windows Python 3.9 is reported in #403 and fixed in #402.

harripj commented 1 year ago

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

hakonanes commented 1 year ago

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?

pc494 commented 1 year ago

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?

still merging this, because @hakonanes is correct that new functionality needs to be saved for 0.11.0

hakonanes commented 1 year ago

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.

pc494 commented 1 year ago

will hold fire on the merge then :)

hakonanes commented 1 year ago

This PR can be merged once checks pass (the single fail is OK)! I plan to release 0.10.2 once it is merged.

hakonanes commented 1 year ago

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.

pyxem / orix