Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#13317 closed enhancement (fixed)

Make species use UniqueRepresentation

Reported by: mhansen Owned by: sage-combinat
Priority: major Milestone: sage-5.11
Component: combinatorics Keywords: species
Cc: Merged in: sage-5.11.beta1
Authors: Mike Hansen Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #5512 Stopgaps:

Status badges

Description


Attachments (1)

trac_13317-species_unique_representation-v2.patch (39.6 KB) - added by chapoton 8 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by mhansen

  • Authors set to Mike Hansen
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by mhansen

Apply trac_13317-species_unique_representation.patch

comment:3 Changed 9 years ago by tscrim

Hey Mike,

Why doesn't GenericCombinatorialSpecies just inherit from UniqueRepresentation, and have just one__classcall__() there (as I recall, this works fine with inheritance, but you can override this as necessary)? If you keep the current code structure, I would move the caching tests in __init__() to the respective __classcall__() as tests.

Also, I recommend the formatting:

Returns the species of singletons.

This species has exactly one structure on a set of size `n`.
It is the same (and is implemented)  as ``CharacteristicSpecies(1)``.

for the __init__() methods.

Finally (and more trivial), trailing whitespace.

Thank you,
Travis

comment:4 Changed 9 years ago by chapoton

  • Keywords species added

comment:5 Changed 9 years ago by chapoton

It rather should be

Returns the species of singletons.

This species has exactly one structure on a set of size `1`.
It is the same (and is implemented)  as ``CharacteristicSpecies(1)``.

comment:6 Changed 9 years ago by chapoton

I have removed the trailing whitespaces. There remains to add doc to the classcall functions.

apply trac_13317-species_unique_representation-v2.patch

comment:7 Changed 9 years ago by chapoton

apply trac_13317-species_unique_representation-v2.patch

comment:8 Changed 9 years ago by chapoton

  • Status changed from needs_review to needs_work
  • Work issues set to doc coverage

comment:9 Changed 8 years ago by chapoton

  • Status changed from needs_work to needs_review

I have added the missing doctests. Needs review

comment:10 Changed 8 years ago by chapoton

Hello there ! The bot has just turned green, so there is an opportunity to have this patch into sage. Anybody for a review ?

comment:11 Changed 8 years ago by mhansen

  • Status changed from needs_review to positive_review

Looks good to me. Thanks!

comment:12 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11
  • Reviewers set to Mike Hansen
  • Work issues doc coverage deleted

comment:13 follow-up: Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 in reply to: ↑ 13 Changed 8 years ago by aschilling

Simon King noticed that the author and reviewer are the same on this patch. Is this legal?

comment:15 Changed 8 years ago by jdemeyer

  • Reviewers changed from Mike Hansen to Frédéric Chapoton
Note: See TracTickets for help on using tickets.