Opened 7 months ago
Closed 7 months ago
#28227 closed defect (fixed)
py3 fixes for weyl_characters.py
Reported by:  jhpalmieri  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  John Palmieri  Reviewers:  Markus Wageringel, Vincent Klein 
Report Upstream:  N/A  Work issues:  
Branch:  7d31fbb (Commits)  Commit:  7d31fbb58883541c4e35d22b8327e1fae8a8ea70 
Dependencies:  Stopgaps: 
Description
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.
Change History (16)
comment:1 Changed 7 months ago by
 Branch set to u/jhpalmieri/py3weylcharacters
comment:2 Changed 7 months ago by
 Commit set to 01793d60a1eb42429a48690742a9cdb6163f43fc
 Status changed from new to needs_review
comment:3 Changed 7 months ago by
 Commit changed from 01793d60a1eb42429a48690742a9cdb6163f43fc to 7f425492104e358eef03189aa8a019cea17ce276
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

comment:4 Changed 7 months ago by
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 breadthfirst search should be unique, should it not?
comment:5 Changed 7 months ago by
There is #27967 already, but it only deals with the graded variant of RecursivelyEnumeratedSet
.
comment:6 followup: ↓ 10 Changed 7 months ago by
+ 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.
comment:7 followup: ↓ 8 Changed 7 months ago by
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 
comment:8 in reply to: ↑ 7 ; followup: ↓ 11 Changed 7 months ago by
Replying to vklein:
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?
comment:9 Changed 7 months ago by
 Commit changed from 7f425492104e358eef03189aa8a019cea17ce276 to 7d31fbb58883541c4e35d22b8327e1fae8a8ea70
comment:10 in reply to: ↑ 6 ; followup: ↓ 12 Changed 7 months ago by
Replying to ghmwageringel:
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 breadthfirst 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 vklein:
+ if key: + fb = sorted(self.basis(), key=key) + else: + fb = list(self.basis())As
str
is the defaultkey
maybe the doc should says that the user can usekey=None
to get an unsorted result.
I added something.
comment:11 in reply to: ↑ 8 Changed 7 months ago by
 Reviewers set to Markus Wageringel, Vincent Klein
Replying to jhpalmieri:
Replying to vklein: ... 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. @ghmwageringel : I let you set the ticket to positive review if you agree.
comment:12 in reply to: ↑ 10 Changed 7 months ago by
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.
comment:13 Changed 7 months ago by
comment:14 Changed 7 months ago by
I think we should merge this ticket now.
From https://trac.sagemath.org/ticket/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.
comment:15 Changed 7 months ago by
 Status changed from needs_review to positive_review
Ok, let us move on here. Setting to positive.
comment:16 Changed 7 months ago by
 Branch changed from u/jhpalmieri/py3weylcharacters to 7d31fbb58883541c4e35d22b8327e1fae8a8ea70
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac 28227: py3 doctest failures in weyl_characters.py