Opened 11 years ago

Closed 10 years ago

#9065 closed enhancement (fixed)

Add support for facade parents

Reported by: nborie Owned by: nthiery
Priority: major Milestone: sage-4.7.1
Component: categories Keywords: facade, parent, TestSuite
Cc: sage-combinat Merged in: sage-4.7.1.alpha0
Authors: Nicolas M. Thiéry Reviewers: Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by nthiery)

The goal of this tickets is to add support for facade parents; see: This thread

The main issue currently is that facade parents (Primes, NonNegativeIntegers, SymmetricFunctions, ...) are not aware that they are, which breaks some of the generic TestSuite tests.

This involves:

  • Adding a new optional argument for Parent.__init__: Parent.__init__(self, facade = [ZZ])
  • Creating a category or abstract class for facade parents
  • Adding a method P.is_parent_of(x) in Sets.ParentMethods which checks that the parent of x is (equal to) P. Override this method for facade parents to check that the parent of x is one of the declared parents of P.
  • Fix P._test_one(), P._test_zero(), P._test_an_element() (and maybe others) to use P.is_parent_of(x) instead of x in P.
  • Updating a couple parents to use it

Attachments (1)

trac_9065-facade_parents-nt.patch (43.5 KB) - added by nthiery 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 11 years ago by hivert

  • Description modified (diff)

comment:3 Changed 10 years ago by nthiery

  • Status changed from new to needs_review

comment:4 Changed 10 years ago by nthiery

  • Authors set to Nicolas M. Thiéry
  • Description modified (diff)

comment:5 follow-up: Changed 10 years ago by hivert

Hi Nicolas,

I started to review this. I currently have only the following interface remark: id there a reason why you need to pass a tuple for parameter facade whereas for category you can pass either a category or a tuple ? For example, why do you force the user to write

Parent.__init__(self, facade = (IntegerRing(), ), category = Sets()) 

instead of

Parent.__init__(self, facade = IntegerRing(), category = Sets()) 

comment:6 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by hivert

Sorry for replying to myself...

I started to review this. I currently have only the following interface remark: id there a reason why you need to pass a tuple for parameter facade whereas for category you can pass either a category or a tuple ?

Actually, it seems that the code in Parent allows for it. So I guess that the example where the one parameter tuple occur could be simplified. I'm writing a review patch on the sage-combinat queue.

comment:7 in reply to: ↑ 6 Changed 10 years ago by nthiery

  • Reviewers set to Florent Hivert

Replying to hivert:

Actually, it seems that the code in Parent allows for it. So I guess that the example where the one parameter tuple occur could be simplified. I'm writing a review patch on the sage-combinat queue.

Good point :-) +1 on that change.

comment:8 follow-up: Changed 10 years ago by nthiery

Hi!

I folded in Florent's reviewer patch for the above issue, added facade option in SearchForest?, updated an example there as well as NN and Primes to be facades, and fixed some trivially failing tests reported by the patchbot. The patch should be good to go. Florent: can you confirm?

comment:9 in reply to: ↑ 8 Changed 10 years ago by hivert

Replying to nthiery:

I folded in Florent's reviewer patch for the above issue, added facade option in SearchForest?, updated an example there as well as NN and Primes to be facades, and fixed some trivially failing tests reported by the patchbot. The patch should be good to go. Florent: can you confirm?

Unfortunately not ! I spotted a few more wrong sphinx markup. Please have a look at my review patch on trac. Otherwise, it is ready to go.

comment:10 Changed 10 years ago by nthiery

  • Status changed from needs_review to positive_review

I checked your patch, folded it in, and reuploaded. Thanks!

comment:11 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1

Changed 10 years ago by nthiery

comment:12 Changed 10 years ago by nthiery

Fixed another trivial failing test in a separate file. Hopefuly the last one!

Florent just checked it, and is ok to leave the positive review.

comment:13 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.