Opened 4 years ago

Closed 4 years ago

#23206 closed enhancement (fixed)

partition_algebra.py: use normal functions instead of functools.partial

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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:

Status badges

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 4 years ago by tscrim

I can take care of this Jeroen.

comment:2 Changed 4 years ago by tscrim

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 4 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Branch set to public/combinat/partition_algebra_use_funcs-23206
  • 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:

52b0b0eHave SetPartitionsX(half)_k be created by actual fucntions.

comment:4 Changed 4 years ago by jdemeyer

  • 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:5 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

doctests...

comment:6 Changed 4 years ago by git

  • Commit changed from 52b0b0ef19240dcad91feeb4c04bf61d638e9886 to 1031aa9ec3fa849c264d976ed8fb72e975f85b50

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

769481cMerge branch 'public/combinat/partition_algebra_use_funcs-23206' of git://trac.sagemath.org/sage into public/combinat/partition_algebra_use_funcs-23206
1031aa9Adding new test function to ensure we still have coverage.

comment:7 Changed 4 years ago by tscrim

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

Version 0, edited 4 years ago by tscrim (next)

comment:8 Changed 4 years ago by jdemeyer

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 4 years ago by tscrim

  • 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 4 years ago by vbraun

  • Branch changed from public/combinat/partition_algebra_use_funcs-23206 to 1031aa9ec3fa849c264d976ed8fb72e975f85b50
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.