orix icon indicating copy to clipboard operation
orix copied to clipboard

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

Open hakonanes opened this issue 2 years ago • 6 comments

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

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 avatar Oct 18 '22 07:10 hakonanes

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

hakonanes avatar Oct 18 '22 07:10 hakonanes

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

harripj avatar Oct 18 '22 07:10 harripj

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?

hakonanes avatar Oct 18 '22 08:10 hakonanes

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

pc494 avatar Oct 18 '22 10:10 pc494

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.

hakonanes avatar Oct 18 '22 11:10 hakonanes

will hold fire on the merge then :)

pc494 avatar Oct 18 '22 11:10 pc494

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 avatar Oct 24 '22 10:10 hakonanes

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.

hakonanes avatar Oct 24 '22 16:10 hakonanes