Restructure Permutation #2067
Restructure Permutation #2067
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2067 +/- ##
==========================================
+ Coverage 84.96% 84.98% +0.02%
==========================================
Files 329 329
Lines 20142 20163 +21
Branches 2525 2528 +3
==========================================
+ Hits 17113 17136 +23
+ Misses 2244 2242 -2
Partials 785 785 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| r""" | ||
| Creates a `Permutation` from an array of preimages of :code:`range(N)`. | ||
| Creates a `Permutation` from either the array of images `permutation_array` or preimages `inverse_permutation_array`. Note that the left action of a permutation on an array `a` is `a[inverse_permutation_array]`. |
There was a problem hiding this comment.
Break up to lines of up to 70-80 characters as per PEP 8. Also, the first tagline should be no more than 1-2 of these lines, because it makes the autogenerated documentation weird. The "Note that" bit can come after an empty line - and probably expanded.
| Args: | ||
| permutation: (deprecated) 1D array listing :math:`g^{-1}(x)` for all :math:`0\le x \le N-1`. | ||
| name: Optional, custom name for the permutation. | ||
| permutation_array: 1D array listing :math:`g(x)` for all :math:`0\le x \le N-1`. | ||
| inverse_permutation_array: 1D array listing :math:`g^{-1}(x)` for all :math:`0\le x \le N-1`. |
There was a problem hiding this comment.
Shoudl mention that you only take one of the three arguments
| arg_list = [permutation, permutation_array, inverse_permutation_array] | ||
| if sum([arg is not None for arg in arg_list]) != 1: | ||
| raise TypeError( | ||
| "Only one among `permutation`, `permutation_array` and `inverse_permutation_array` must be specified." |
There was a problem hiding this comment.
| "Only one among `permutation`, `permutation_array` and `inverse_permutation_array` must be specified." | |
| "Precisely one of `permutation`, `permutation_array`,\n" | |
| "and `inverse_permutation_array` must be specified." |
not specifying any of them is also bad
|
|
||
| if permutation is not None: | ||
| warn_deprecation( | ||
| "The argument `permutation` is deprecated. In order to clarify notations, you should either pass the array of images `permutation_array` or preimages `inverse_permutation_array`." |
There was a problem hiding this comment.
| "The argument `permutation` is deprecated. In order to clarify notations, you should either pass the array of images `permutation_array` or preimages `inverse_permutation_array`." | |
| "The argument `permutation` is deprecated.\n\n" | |
| "For clarity, you should either pass the array of images\n" | |
| "`permutation_array` or preimages `inverse_permutation_array`.\n" | |
| "The latter matches the the behaviour of `permutation`." |
|
|
||
| @deprecated("Deprecated in favor of `permutation.inverse_permutation_array`") |
There was a problem hiding this comment.
I think there's an argument specifically for deprecating A in favour of B
|
|
||
| arg_list = [permutation, permutation_array, inverse_permutation_array] | ||
| if sum([arg is not None for arg in arg_list]) != 1: | ||
| raise TypeError( |
There was a problem hiding this comment.
Isn't this a ValueError?
| Returns: | ||
| a `Permutation` object encoding the same permutation | ||
| A `Permutation` object encoding the same permutation. |
There was a problem hiding this comment.
| A `Permutation` object encoding the same permutation. | |
| A `Permutation` object that encodes the same permutation. |
I'd be cautious with deprecating this because it can be used in other contexts.
Yes, we need this so |
The clarification of names in
Permutationas discussed with @PhilipVinc and @attila-i-szabo. The objective is to solve the confusion that might be caused by the propertypermutationactually encoding the inverse permutation.Changes:
Permutationnow takes in either the permutation array or its inverse.nameis now a keyword argument.Permutationpropertypermutationis deprecated in favor ofpermutation_arrayandinverse_permutation_array.__repr__now returns the array of the permutation instead of its inverse.__array__is deprecated and replaced byPermutation.inverse_permutation_arrayfor more clarity.As far as I can tell, the PR is functional right now, but I think it's better to wait for Attila's PR to be merged so we can update the usage of the
Permutationclass inSpaceGroupand the likes, and avoid an avalanche of deprecation warnings.Two other points:
apply_to_idinPermutation. From what I can see, it's only used twice inside the tests to obtain the permutation (instead of its inverse). Since it's effectively identical top.permutation_array[x]now, would it make sense to deprecate it?HashableArraybusiness going on, and I'm not sure what it's for (being able to hashpermutationI guess?). I think what I wrote in that regard makes sense, but you should probably check.