Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for facade parents #9065

Closed
sagetrac-nborie mannequin opened this issue May 27, 2010 · 13 comments
Closed

Add support for facade parents #9065

sagetrac-nborie mannequin opened this issue May 27, 2010 · 13 comments

Comments

@sagetrac-nborie
Copy link
Mannequin

sagetrac-nborie mannequin commented May 27, 2010

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

CC: @sagetrac-sage-combinat

Component: categories

Keywords: facade, parent, TestSuite

Author: Nicolas M. Thiéry

Reviewer: Florent Hivert

Merged: sage-4.7.1.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/9065

@sagetrac-nborie sagetrac-nborie mannequin added this to the sage-4.7 milestone May 27, 2010
@sagetrac-nborie sagetrac-nborie mannequin assigned nthiery May 27, 2010
@nthiery

This comment has been minimized.

@hivert

This comment has been minimized.

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented Mar 28, 2011

Author: Nicolas M. Thiéry

@hivert
Copy link

hivert commented Apr 4, 2011

comment:5

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()) 

@hivert
Copy link

hivert commented Apr 4, 2011

comment:6

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.

@nthiery
Copy link
Contributor

nthiery commented Apr 5, 2011

comment:7

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.

@nthiery
Copy link
Contributor

nthiery commented Apr 5, 2011

Reviewer: Florent Hivert

@nthiery
Copy link
Contributor

nthiery commented Apr 21, 2011

comment:8

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?

@hivert
Copy link

hivert commented Apr 22, 2011

comment:9

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.

@nthiery
Copy link
Contributor

nthiery commented Apr 23, 2011

comment:10

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

@nthiery
Copy link
Contributor

nthiery commented Apr 26, 2011

comment:12

Attachment: trac_9065-facade_parents-nt.patch.gz

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.

@jdemeyer
Copy link

jdemeyer commented May 3, 2011

Merged: sage-4.7.1.alpha0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants