py3 fixes for weyl_characters.py

[jhpalmieri]

Fix the Python 3 doctest failures in combinat/root_systems/weyl_characters.py. The failures all come down to sorting. The more complicated problem is the method fusion_labels which relies on sorted output when it shouldn't.

Component: python3

Author: John Palmieri

Branch/Commit: 7d31fbb

Reviewer: Markus Wageringel, Vincent Klein

Issue created by migration from https://trac.sagemath.org/ticket/28227

[jhpalmieri]

Branch: u/jhpalmieri/py3-weyl-characters

[jhpalmieri]

New commits:

01793d6

trac 28227: py3 doctest failures in weyl_characters.py

[jhpalmieri]

Commit: 01793d6

[sagetrac-git]

Changed commit from 01793d6 to 7f42549

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7f42549

trac 28227: py3 doctest failures in weyl_characters.py

[mwageringel]

comment:4

It would be clearer if the second test here makes use of the sorted basis instead of sorting the highest weights, assuming assigning labels in the third test takes those weights into account.

        sage: sorted(B22.basis(), key=str)
        [B22(0,0), B22(0,1), B22(0,2), B22(1,0), B22(1,1), B22(2,0)]
        sage: sorted([x.highest_weight() for x in B22.basis()], key=str)
        [(0, 0), (1, 0), (1, 1), (1/2, 1/2), (2, 0), (3/2, 1/2)]
        sage: B22.fusion_labels(['1','X','Y1','Y2','Xp','Z'])

Also, I am wondering whether the more fundamental problem is that RecursivelyEnumeratedSet used for the basis does not give consistent results.

sage: S = RecursivelyEnumeratedSet([1000], lambda a: [a//2, a//3]); S
A recursively enumerated set (breadth first search)
sage: str(list(S))  # py3
'[1000, 500, 333, 250, 166, 111, 37, 83, 125, 55, 41, 12, 18, 27, 62, 4, 6, 9, 13, 20, 31, 1, 2, 3, 10, 15, 0, 5, 7]'
sage: str(list(S))  # py2
'[1000, 500, 333, 250, 166, 111, 37, 83, 125, 55, 41, 18, 27, 12, 62, 4, 6, 9, 13, 20, 31, 1, 2, 3, 10, 15, 0, 5, 7]'

If the successor function gives consistent results, the breadth-first search should be unique, should it not?

[mwageringel]

comment:5

There is #27967 already, but it only deals with the graded variant of RecursivelyEnumeratedSet.

[vinklein]

comment:6

+        if key:
+            fb = sorted(self.basis(), key=key)
+        else:
+            fb = list(self.basis())

As str is the default key maybe the doc should says that the user can use key=None to get an unsorted result.

[vinklein]

comment:7

With this branch and py2 i get the following error :

sage -t --long src/sage/combinat/root_system/weyl_characters.py
**********************************************************************
File "src/sage/combinat/root_system/weyl_characters.py", line 2233, in sage.combinat.root_system.weyl_characters.FusionRing
Failed example:
    Z*Z
Expected:
    1
Got:
    1 + Y1 + Y2
**********************************************************************
1 item had failures:
   1 of  14 in sage.combinat.root_system.weyl_characters.FusionRing
    [308 tests, 1 failure, 1.88 s]
----------------------------------------------------------------------
sage -t --long src/sage/combinat/root_system/weyl_characters.py  # 1 doctest failed
----------------------------------------------------------------------

[jhpalmieri]

comment:8

Replying to @vinklein:

With this branch and py2 i get the following error :

sage -t --long src/sage/combinat/root_system/weyl_characters.py
**********************************************************************
File "src/sage/combinat/root_system/weyl_characters.py", line 2233, in sage.combinat.root_system.weyl_characters.FusionRing
Failed example:
    Z*Z
Expected:
    1
Got:
    1 + Y1 + Y2
**********************************************************************
1 item had failures:
   1 of  14 in sage.combinat.root_system.weyl_characters.FusionRing
    [308 tests, 1 failure, 1.88 s]
----------------------------------------------------------------------
sage -t --long src/sage/combinat/root_system/weyl_characters.py  # 1 doctest failed
----------------------------------------------------------------------

I can only reproduce this if I check out the branch and fail to run make or ./sage -b before running tests. Can you double check that you really get this failure?

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

859549e	trac 28227: more documentation
7d31fbb	trac 28227: use more consistent sorting throughout doctest

[sagetrac-git]

Changed commit from 7f42549 to 7d31fbb

[jhpalmieri]

comment:10

Replying to @mwageringel:

It would be clearer if the second test here makes use of the sorted basis instead of sorting the highest weights, assuming assigning labels in the third test takes those weights into account.

        sage: sorted(B22.basis(), key=str)
        [B22(0,0), B22(0,1), B22(0,2), B22(1,0), B22(1,1), B22(2,0)]
        sage: sorted([x.highest_weight() for x in B22.basis()], key=str)
        [(0, 0), (1, 0), (1, 1), (1/2, 1/2), (2, 0), (3/2, 1/2)]
        sage: B22.fusion_labels(['1','X','Y1','Y2','Xp','Z'])

Is this better?

Also, I am wondering whether the more fundamental problem is that RecursivelyEnumeratedSet used for the basis does not give consistent results.

sage: S = RecursivelyEnumeratedSet([1000], lambda a: [a//2, a//3]); S
A recursively enumerated set (breadth first search)
sage: str(list(S))  # py3
'[1000, 500, 333, 250, 166, 111, 37, 83, 125, 55, 41, 12, 18, 27, 62, 4, 6, 9, 13, 20, 31, 1, 2, 3, 10, 15, 0, 5, 7]'
sage: str(list(S))  # py2
'[1000, 500, 333, 250, 166, 111, 37, 83, 125, 55, 41, 18, 27, 12, 62, 4, 6, 9, 13, 20, 31, 1, 2, 3, 10, 15, 0, 5, 7]'

If the successor function gives consistent results, the breadth-first search should be unique, should it not?

Should we delay the changes here until #27967 is resolved, or do these and then perhaps have to change then again depending on #27967 and/or other changes to RecursiveEnumeratedSet?

Replying to @vinklein:

+        if key:
+            fb = sorted(self.basis(), key=key)
+        else:
+            fb = list(self.basis())

As str is the default key maybe the doc should says that the user can use key=None to get an unsorted result.

I added something.

[vinklein]

Reviewer: Markus Wageringel, Vincent Klein

[vinklein]

comment:11

Replying to @jhpalmieri:

Replying to @vinklein:
...
I can only reproduce this if I check out the branch and fail to run make or ./sage -b before running tests. Can you double check that you really get this failure?

I had a mess in my py2 local branch. It works now.

I'm ok with this ticket as it is. @mwageringel : I let you set the ticket to positive review if you agree.

[mwageringel]

comment:12

Replying to @jhpalmieri:

Is this better?

Yes, thanks.

Should we delay the changes here until #27967 is resolved, or do these and then perhaps have to change then again depending on #27967 and/or other changes to RecursiveEnumeratedSet?

IMO, it would be better to delay this as it is a change of the API, not just the doctests. I am not sure where #27967 is headed though, so I will not insist. Feel free to set this to positive if you prefer these changes be merged now.

[jhpalmieri]

comment:13

If you think that #27967 will take a while to get resolved, then maybe we should merge these changes now to fix these doctests, but the two of you (vklein, gh-mwageringel) have a better idea of the fate of #27967 than I do, so I will let you decide what to do here.

[vinklein]

comment:14

I think we should merge this ticket now.

From #27967 comment:10

Also, why should the enumeration order be certified? This is not claimed in the documentation.

I think this is a valid point and therefore modifying RecursivelyEnumeratedSet rather than the classes using it is debatable. So yes i think #27967 will take a while.

[mwageringel]

comment:15

Ok, let us move on here. Setting to positive.

[vbraun]

Changed branch from u/jhpalmieri/py3-weyl-characters to 7d31fbb