Opened 3 years ago

Closed 3 years ago

#24485 closed enhancement (fixed)

get rid of CombinatorialClass in Necklaces

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.2
Component: combinatorics Keywords:
Cc: tscrim Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: cb716c9 (Commits) Commit: cb716c93d3aab753e4701ccd209a9f58fbe7a728
Dependencies: Stopgaps:

Description

as part of #12913

Change History (6)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/24485
  • Commit set to fbecb41c541d81ff1bf963fe3deadf8bc125c6e9
  • Status changed from new to needs_review

New commits:

​fbecb41refresh the necklaces (no more CombinatorialClass)

comment:2 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Two things: __classcall_private__ needs a doctest: 

sage: Necklaces([2,1,1]) is Necklaces(Composition([2,1,1]))
True

I think you should just (attempt to) convert into a Composition when content is not a Composition.

Also, I was told at some point that it is better to have UniqueRepresentation be before Parent, but I don't remember precisely why (and don't feel like you have to change it).

comment:3 Changed 3 years ago by git

  • Commit changed from fbecb41c541d81ff1bf963fe3deadf8bc125c6e9 to cb716c93d3aab753e4701ccd209a9f58fbe7a728

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

​cb716c9details

comment:4 Changed 3 years ago by chapoton

thanks. done

comment:5 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:6 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/24485 to cb716c93d3aab753e4701ccd209a9f58fbe7a728
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.