Opened 5 years ago
Closed 5 years ago
#23206 closed enhancement (fixed)
partition_algebra.py: use normal functions instead of functools.partial
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  combinatorics  Keywords:  
Cc:  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  1031aa9 (Commits, GitHub, GitLab)  Commit:  1031aa9ec3fa849c264d976ed8fb72e975f85b50 
Dependencies:  Stopgaps: 
Description
Various functions from src/sage/combinat/partition_algebra.py
are implemented as functools.partial
for no obvious reason. Then a __doc__
attribute is assigned, which will break with the doctest framework changes in #23196.
Change History (10)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
There is a reason why this was done: to remove the boilerplate code of checking integer or n.5
and to delegate to the correct class. However, this is not a very good approach.
comment:3 Changed 5 years ago by
 Branch set to public/combinat/partition_algebra_use_funcs23206
 Commit set to 52b0b0ef19240dcad91feeb4c04bf61d638e9886
 Status changed from new to needs_review
Here is a version with as minimal boilerplate code as I could manage. I also did some trivial cleanup of the doc of these functions.
New commits:
52b0b0e  Have SetPartitionsX(half)_k be created by actual fucntions.

comment:4 Changed 5 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
For some reason, combinat
developers really like to write docstrings in strange ways...
comment:6 Changed 5 years ago by
 Commit changed from 52b0b0ef19240dcad91feeb4c04bf61d638e9886 to 1031aa9ec3fa849c264d976ed8fb72e975f85b50
comment:7 Changed 5 years ago by
 Status changed from needs_work to needs_review
So we no longer have anything in Sage that uses that idiom, so I made a new file in tests
specifically for that. I didn't copy the test over because I didn't want to duplicate it and moving it felt unnatural.
comment:8 Changed 5 years ago by
I think it would have been fine to just remove that test.
But now that you changed the doctest, let's keep it.
comment:9 Changed 5 years ago by
 Status changed from needs_review to positive_review
I thought about that, but I didn't want to remove doctest coverage for a feature we had a trac ticket explicitly for.
The patchbot comes back essentially green, and so I'm taking your comments as a positive review.
comment:10 Changed 5 years ago by
 Branch changed from public/combinat/partition_algebra_use_funcs23206 to 1031aa9ec3fa849c264d976ed8fb72e975f85b50
 Resolution set to fixed
 Status changed from positive_review to closed
I can take care of this Jeroen.