SignedPermutation should allow iterables as input

[Sandstorm831]

This is my pull request originally which addresses the issue #34923
Fixes #34923

[mantepse]

I don't understand the reason for the try clause. Wouldn't just pi = list(pi) do? I think that's what's done in the case of permutations.

[mantepse]

Ping?

[Sandstorm831]

Sorry sir, I was away for some time. Actually I have used try and except just to point error at that moment only rather than calling error on further calls, but yes it is definitely not necessary.

[codecov-commenter]

Codecov Report

Base: 88.60% // Head: 88.59% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (ba4165f) compared to base (c000c95).
Patch coverage: 83.72% of modified lines in pull request are covered.

❗ Current head ba4165f differs from pull request most recent head 0e9d4fc. Consider uploading reports for the commit 0e9d4fc to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #34974      +/-   ##
===========================================
- Coverage    88.60%   88.59%   -0.01%     
===========================================
  Files         2136     2140       +4     
  Lines       396142   396963     +821     
===========================================
+ Hits        350990   351698     +708     
- Misses       45152    45265     +113     

Impacted Files	Coverage Δ
src/sage/combinat/colored_permutations.py	95.76% <83.72%> (-0.31%)	⬇️
src/sage/groups/additive_abelian/qmodnz.py	91.48% <0.00%> (-2.13%)	⬇️
src/sage/modular/local_comp/liftings.py	98.33% <0.00%> (-1.67%)	⬇️
src/sage/cpython/_py2_random.py	74.79% <0.00%> (-1.66%)	⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py	94.09% <0.00%> (-1.58%)	⬇️
src/sage/modular/hecke/algebra.py	94.65% <0.00%> (-1.07%)	⬇️
src/sage/homology/matrix_utils.py	87.15% <0.00%> (-0.92%)	⬇️
src/sage/combinat/posets/poset_examples.py	87.67% <0.00%> (-0.86%)	⬇️
src/sage/coding/channel.py	97.10% <0.00%> (-0.73%)	⬇️
src/sage/groups/generic.py	88.34% <0.00%> (-0.68%)	⬇️
... and 68 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mantepse]

So, could you please fix the pull request? Please make sure that

sage: SignedPermutation(range(1,4))

and

sage: SignedPermutations(3)(range(1,4))

both work. I don't think you should catch the exception.

BTW, there is a bug in permutation.py:

sage: pi = Permutations(3)(range(5,0, -1)); pi
[5, 4, 3, 2, 1]
sage: pi.parent()
Standard permutations of 3

The code in SignedPermutations._element_constructor_ also looks fishy, but that's for another ticket, I'd say.

[tscrim]

It makes no sense to generate an iterator that you never use. It also does not fix the underlying problem. You need to change the _element_constructor_ to handle something other than a list. Although it is good to create a list that you use in the _classcall_private_.

[Sandstorm831]

So, could you please fix the pull request? Please make sure that

sage: SignedPermutation(range(1,4))

and

sage: SignedPermutations(3)(range(1,4))

both work. I don't think you should catch the exception.

BTW, there is a bug in permutation.py:

sage: pi = Permutations(3)(range(5,0, -1)); pi
[5, 4, 3, 2, 1]
sage: pi.parent()
Standard permutations of 3

The code in SignedPermutations._element_constructor_ also looks fishy, but that's for another ticket, I'd say.

Sir, but doesn't that applying just the list function beforehand will also raise the same issue of giving an exception in

sage: SignedPermutations(3)(range(1,4))

[Sandstorm831]

It makes no sense to generate an iterator that you never use. It also does not fix the underlying problem. You need to change the _element_constructor_ to handle something other than a list. Although it is good to create a list that you use in the _classcall_private_.

Sorry sir, but I didn't well understood your point and what needs to be joined.

[tscrim]

All you are doing is hiding the problem instead of actually fixing it by changing the _element_constructor_ as @mantepse was saying (it actually is the issue of this ticket). I completely agree with everything he said. There’s no reason to trap any exceptions there.

[Sandstorm831]

Ok, I got your point, let me look into it more thoroughly

[Sandstorm831]

Thank You @tscrim and @mantepse for your detailed review, I was going in wrong direction, there is small big in _element_constructor_ only.
I have made some changes, and now all the cases are running fine

sage: S = SignedPermutations(3)(range(1,4)); S
[1, 2, 3]
sage: S = SignedPermutation(range(1,4)); S
[1, 2, 3]

Please look at my changes, and please tell if there anything else needs to be done.

[mantepse]

Sorry, no, this is not the correct fix. Not every iterable is a range object

[tscrim]

Try casting the input to a list if it is not an instance of a list or self.element_class.

[tscrim]

That won't work because an iterable is not something that is guaranteed to have a __getitem__. Please follow the suggestion about checking the type of the input and then casting the input to a list when appropriate. It is not just a matter of changing the type of input in the _element_constructor_.

[Sandstorm831]

I have checked the type of input, it is same as we passing, that is "range". Although I have tried converting the input to list, but it is throwing some error on that, so thought of this.
I have also thought of using iter() function to determine if the object is iterable like i have done using a try except block, Would that work ??

[tscrim]

range definitely does not throw and error when converting it to a list. In fact, things that should error out will (modulo crazy input like iter(ZZ), which is impossible to check against).

What I am saying is very simple code that would go like this for input x:

if isinstance(x, self.element_class) and x.parent() is self:
    return self
x = list(x)
# The rest of the code assuming it is a list

A similar change should be done for ColoredPermutations as well...

[Sandstorm831]

 if isinstance(x, self.element_class) and x.parent() is self:
     return self

May I ask why we have to check if x is an instance of self.element_class

[tscrim]

 if isinstance(x, self.element_class) and x.parent() is self:
     return self

May I ask why we have to check if x is an instance of self.element_class

Speed. You don't need to do anything to the input if it is already an element of self. We can also do something slightly better for the same element class, but not necessarily the same parent. However, that can be a followup. Plus not every input type has a parent() method (e.g., a list).

[Sandstorm831]

Thank you @tscrim for your reply, I have made the necessary changes. Please tell if there any other changes to be done.

[tscrim]

You also need to add doctests showing that the issue is fixed. Not only with range, but also a tuple and a signed/colored permutation.

[Sandstorm831]

@tscrim sir, I have added doctests, please tell if there is anything that I have missed, or else that needs to be done.

[tscrim]

You also need to add doctests for colored permutations.

[Sandstorm831]

@tscrim Sir, I have tested colored permutations that whenever we passed self.element_class() as input to ColoredPermutations It doesn't call _element_constructor_() but deals internally return self automatically. So I have removed that code from _element_constructor_().
Also, remaining I have added the doctest for case of tuple, and a range of tuple is not possible so that would not be passes anyway. Although if we pass a tuple earlier than as expected, earlier it would have ignored which have been fixed now.
Please tell if anything else needs to be done in it.

[tscrim]

@tscrim Sir, I have tested colored permutations that whenever we passed self.element_class() as input to ColoredPermutations It doesn't call _element_constructor_() but deals internally return self automatically. So I have removed that code from _element_constructor_().

This isn't always the case. It should be, but I have come across this from time to time. Please listen to people with some experience.

The other thing that should be done is signed permutations should handle 2-colored permutations as a special case and vice versa.

Also, remaining I have added the doctest for case of tuple, and a range of tuple is not possible so that would not be passes anyway.

You should add a doctest for signed and colored permutations as input as well, as I said previously.

Although if we pass a tuple earlier than as expected, earlier it would have ignored which have been fixed now. Please tell if anything else needs to be done in it.

This makes no sense.

[tscrim]

The other thing that should be done is signed permutations should handle 2-colored permutations as a special case and vice versa.

Actually, this is not necessary because of the coercion map. Although only one of them should be a coercion map, the other should be a conversion through _element_constructor_. That can be done separately.

[Sandstorm831]

This isn't always the case. It should be, but I have come across this from time to time. Please listen to people with some experience.

Sorry sir for my mistake. As during testing, I was unable to find any case where _element_constructor_ is called, so I removed it. Apologies for my mistake.

Actually, this is not necessary because of the coercion map. Although only one of them should be a coercion map, the other should be a conversion through _element_constructor_. That can be done separately.

So does it mean that I don't have to add anything as one of the argument is handled through coercion map and other by _element_constructor, or we have to add code for the same.

[tscrim]

Actually, this is not necessary because of the coercion map. Although only one of them should be a coercion map, the other should be a conversion through _element_constructor_. That can be done separately.

So does it mean that I don't have to add anything as one of the argument is handled through coercion map and other by _element_constructor, or we have to add code for the same.

No, nothing needs to be added for these cases.

[github-actions]

Documentation preview for this PR is ready! 🎉
Built with commit: 0e9d4fc

[tscrim]

Thank you. This is good with me. @mantepse Let me know if there is something you want changed, and we can revert.

[mantepse]

Thank you. This is good with me. @mantepse Let me know if there is something you want changed, and we can revert.

Thank you, both! I only see two extremely minor possible improvements (i.e., I am not sure whether they actually would be improvements), which certainly should not hold up the ticket, so I only mention them here. My next, more real, concern is #34925 (comment).

One remark: the error messages are apparently not tested.

diff --git a/src/sage/combinat/colored_permutations.py b/src/sage/combinat/colored_permutations.py
index 28b20e502af..0ef0575a36d 100644
--- a/src/sage/combinat/colored_permutations.py
+++ b/src/sage/combinat/colored_permutations.py
@@ -436,7 +436,7 @@ class ColoredPermutations(Parent, UniqueRepresentation):
         [[0, 1, 0], [1, 2, 3]]
 
     We can also create a colored permutation by passing
-    an iterable consisting of tuples consisting of ``(color, element)``::
+    an iterable consisting of tuples ``(color, element)``::
 
         sage: x = C([(2,1), (3,3), (3,2)]); x
         [[2, 3, 3], [1, 3, 2]]
@@ -685,8 +685,8 @@ class ColoredPermutations(Parent, UniqueRepresentation):
 
         INPUT:
 
-        Either a list of pairs (color, element)
-        or a pair of lists (colors, elements).
+        Either a list of pairs ``(color, element)``
+        or a pair of lists ``(colors, elements)``.
 
         TESTS::
 
@@ -700,13 +700,11 @@ class ColoredPermutations(Parent, UniqueRepresentation):
             return self
         x = list(x)
         if isinstance(x[0], tuple):
-            c = []
-            p = []
-            for k in x:
-                if len(k) != 2:
-                    raise ValueError("input must be pairs (color, element)")
-                c.append(self._C(k[0]))
-                p.append(k[1])
+            try:
+                c = [self._C(c) for c, _ in x]
+                p = [e for _, e in x]
+            except ValueError:
+                raise ValueError("input must be pairs (color, element)")
             return self.element_class(self, c, self._P(p))
 
         if len(x) != 2:
@@ -1383,28 +1381,19 @@ class SignedPermutations(ColoredPermutations):
             return self
         x = list(x)
         if x and isinstance(x[0], tuple):
-            c = []
-            p = []
-            for k in x:
-                if len(k) != 2:
-                    raise ValueError("input must be pairs (sign, element)")
-                if k[0] != 1 and k[0] != -1:
-                    raise ValueError("the sign must be +1 or -1")
-                c.append(ZZ(k[0]))
-                p.append(k[1])
+            try:
+                c = [ZZ(e) for e, _ in x]
+            except ValueError:
+                raise ValueError("input must be pairs (sign, element)")
+            if any(e != 1 and e != -1 for e in c):
+                raise ValueError("the sign must be +1 or -1")
+            p = [e for _, e in x]
             return self.element_class(self, c, self._P(p))
 
         if len(x) == self._n:
-            c = []
-            p = []
             one = ZZ.one()
-            for v in x:
-                if v > 0:
-                    c.append(one)
-                    p.append(v)
-                else:
-                    c.append(-one)
-                    p.append(-v)
+            c = [one if e > 0 else -one for e in x]
+            p = [e if e > 0 else -e for e in x]
             return self.element_class(self, c, self._P(p))
 
         if len(x) != 2:

[Sandstorm831]

@mantepse sir, so should I make these above changes ?

[mantepse]

@mantepse sir, so should I make these above changes ?

I leave that decision to Travis.

[tscrim]

@mantepse I will take care of it on a followup PR. I see some other improvements that I want to do anyways (e.g., having only one coercion map and making the other a conversion).