Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#2328 closed defect (fixed)

[with patch, positive review] many docstrings in combinat functions are unhelpful, outdated, or wrong

Reported by: ddrake Owned by: mhansen
Priority: minor Milestone: sage-2.10.4
Component: combinatorics Keywords:
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This link has my original complaints. On IRC, the consensus was that the uncapitalized versions of these functions should not be used, and may be deprecated in the future. The documentation for such functions should be updated to reflect this, and the documentation for other functions should be improved as well.

Attachments (4)

8710.patch (2.9 KB) - added by wdj 14 years ago.
combinat-doc.patch (11.3 KB) - added by ddrake 14 years ago.
combinat-doc.2.patch (10.8 KB) - added by mhansen 14 years ago.
2328.patch (14.5 KB) - added by mhansen 14 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 14 years ago by wdj

The attached patch fixes a few of the docstring problems mentioned by Dan Drake. Really very minor changes. I didn't touch the functions written by Mike Hansen since I wasn't sure what plans he had for them. The issue of the lower case vs upper case functions wasn't dealt with. Perhaps the lower case functions should be removed from combinat/all.py?

Changed 14 years ago by wdj

comment:2 in reply to: ↑ 1 Changed 14 years ago by ddrake

Replying to wdj:

Perhaps the lower case functions should be removed from combinat/all.py?

They should first be marked deprecated, and later removed. The exact process for doing this is not yet determined; there was some discussion on sage-devel in January. Until more has been decided on that front, I think we should just say in the docstrings "this function will be deprecated in the future, and eventually removed; use Foo.whatever() instead".

Changed 14 years ago by ddrake

comment:3 Changed 14 years ago by ddrake

  • Summary changed from many docstrings in combinat functions are unhelpful, outdated, or wrong to many docstrings in combinat functions are unhelpful, outdated, or wrong [with patch, needs review]

I've attached a patch which addresses most of my complaining in the email thread. :)

It's against 2.10.2 with mhansen's 2432 patch and wdj's 8710 patch applied.

Changed 14 years ago by mhansen

comment:4 Changed 14 years ago by mhansen

  • Summary changed from many docstrings in combinat functions are unhelpful, outdated, or wrong [with patch, needs review] to [with patch, needs review] many docstrings in combinat functions are unhelpful, outdated, or wrong

I've uploaded combinat-doc.2.patch which replaces the first combinat-doc.patch

comment:5 follow-up: Changed 14 years ago by wdj

I had trouble applying combinat-doc.2.patch against 2.10.2, 2.10.3.rc2 and 2.10.rc3, so I don't know what this is supposed to apply against.

comment:6 in reply to: ↑ 5 Changed 14 years ago by ddrake

Replying to wdj:

I had trouble applying combinat-doc.2.patch against 2.10.2, 2.10.3.rc2 and 2.10.rc3, so I don't know what this is supposed to apply against.

My patch was against 2.10.2 + your 8710 patch + the #2432 patch...AFAIK mhansen just fiddled with a couple commented lines to get combinat-doc.2.patch.

comment:7 Changed 14 years ago by mabshoff

  • Milestone set to sage-2.10.4

comment:8 Changed 14 years ago by mhansen

I've looked over the patches and give this a positive review.

Since my patch addressing the referee's concerns for #2432 touches a lot of things, I'll build a patch with the changes in #2328 to be included with #2432.

comment:9 Changed 14 years ago by rlm

  • Milestone changed from sage-2.11 to sage-2.10.4

comment:10 Changed 14 years ago by mhansen

  • Summary changed from [with patch, needs review] many docstrings in combinat functions are unhelpful, outdated, or wrong to [with patch, positive review] many docstrings in combinat functions are unhelpful, outdated, or wrong

Changed 14 years ago by mhansen

comment:11 Changed 14 years ago by mhansen

Apply only 2328.patch after #2432 is applied.

comment:12 Changed 14 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 2.10.4.alpha0

comment:13 Changed 13 years ago by nthiery

  • Cc sage-combinat added
Note: See TracTickets for help on using tickets.