Opened 4 months ago

Closed 4 months ago

#28227 closed defect (fixed)

py3 fixes for weyl_characters.py

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-8.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 4 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/py3-weyl-characters

comment:2 Changed 4 months ago by jhpalmieri

  • Commit set to 01793d60a1eb42429a48690742a9cdb6163f43fc
  • Status changed from new to needs_review

New commits:

01793d6trac 28227: py3 doctest failures in weyl_characters.py

comment:3 Changed 4 months ago by git

  • Commit changed from 01793d60a1eb42429a48690742a9cdb6163f43fc to 7f425492104e358eef03189aa8a019cea17ce276

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

7f42549trac 28227: py3 doctest failures in weyl_characters.py

comment:4 Changed 4 months ago by gh-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'])

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?

comment:5 Changed 4 months ago by gh-mwageringel

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

comment:6 follow-up: Changed 4 months ago by vklein

+        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 follow-up: Changed 4 months ago by 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
----------------------------------------------------------------------

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 months ago by jhpalmieri

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 4 months ago by git

  • Commit changed from 7f425492104e358eef03189aa8a019cea17ce276 to 7d31fbb58883541c4e35d22b8327e1fae8a8ea70

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

859549etrac 28227: more documentation
7d31fbbtrac 28227: use more consistent sorting throughout doctest

comment:10 in reply to: ↑ 6 ; follow-up: Changed 4 months ago by jhpalmieri

Replying to gh-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 vklein:

+        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.

comment:11 in reply to: ↑ 8 Changed 4 months ago by vklein

  • 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. @gh-mwageringel : I let you set the ticket to positive review if you agree.

comment:12 in reply to: ↑ 10 Changed 4 months ago by gh-mwageringel

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 4 months ago by jhpalmieri

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.

comment:14 Changed 4 months ago by vklein

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 4 months ago by gh-mwageringel

  • Status changed from needs_review to positive_review

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

comment:16 Changed 4 months ago by vbraun

  • Branch changed from u/jhpalmieri/py3-weyl-characters to 7d31fbb58883541c4e35d22b8327e1fae8a8ea70
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.