Opened 12 years ago
Closed 11 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: |
Description (last modified by )
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)
inSets.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 useP.is_parent_of(x)
instead ofx in P
. - Updating a couple parents to use it
Attachments (1)
Change History (14)
comment:1 Changed 12 years ago by
- Description modified (diff)
comment:2 Changed 12 years ago by
- Description modified (diff)
comment:3 Changed 11 years ago by
- Status changed from new to needs_review
comment:4 Changed 11 years ago by
- Description modified (diff)
comment:5 follow-up: ↓ 6 Changed 11 years ago by
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 11 years ago by
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 forcategory
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 11 years ago by
- 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: ↓ 9 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
- Status changed from needs_review to positive_review
I checked your patch, folded it in, and reuploaded. Thanks!
comment:11 Changed 11 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
Changed 11 years ago by
comment:12 Changed 11 years ago by
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 11 years ago by
- Merged in set to sage-4.7.1.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
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 forcategory
you can pass either a category or a tuple ? For example, why do you force the user to writeinstead of