Opened 12 years ago
Closed 7 years ago
#10194 closed enhancement (fixed)
Set factories
Reported by:  Nicolas M. Thiéry  Owned by:  Florent Hivert 

Priority:  major  Milestone:  sage6.8 
Component:  combinatorics  Keywords:  factories, days30, Cernay2012, days57 
Cc:  Sage Combinat CC user, Florent Hivert  Merged in:  
Authors:  Florent Hivert  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  763f93c (Commits, GitHub, GitLab)  Commit:  763f93c95ecf8e79ff48339491212a6417d2c4f8 
Dependencies:  Stopgaps: 
Description (last modified by )
At Sage days 30, a long brainstorm seems to have finalized the design. Here is a excerpt from the documentation:
A set factory F
is device, whose goal is to construct parent P
which models subsets of a big set S
. Typically, the P
s are constructed
within families obtained by putting a bunch of constraints cons
on the
elements of the set S
. In such a hierarchy of subsets, one needs to have a
fine and easy control on the elements construction. That is, one often needs
that P
constructs elements in a subclass of its usual class for element. On
the contrary, one also often needs P
to be a facade parent, meaning that P
construct element whose actual parent is not P
itself.
The role of a set factory is twofold:
 manage a database of constructors for the different parents
P = F(cons)
depending on the various kinds of constraintscons
. Note: currently there is no real support for that. We are gathering use case before fixing the interface.
 ensure that the elements
e = P(...)
created by the different parents follows a consistent policy concerning their class and parent.
The patch implement this idea while trying to leave as much as possible space
for further improvement. In particular, I tried to specify the few possible
things about constraints. I'm even not completely sure about add_constraints
.
Please comment and review.
Florent
Attachments (2)
Change History (69)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Description:  modified (diff) 

Owner:  changed from Sage Combinat CC user to Florent Hivert 
Summary:  Enumerated set factories → Set factories 
comment:3 Changed 12 years ago by
Keywords:  factories days30 added 

comment:4 Changed 11 years ago by
Keywords:  Cernay2012 added 

comment:5 Changed 11 years ago by
Milestone:  → sage5.0 

comment:6 Changed 11 years ago by
Description:  modified (diff) 

Status:  new → needs_review 
Changed 11 years ago by
Attachment:  trac_10194factories_policyfh.patch added 

comment:7 followup: 8 Changed 11 years ago by
Status:  needs_review → needs_work 

Work issues:  → doctest failures 
This fails doctests on every version tested  see patchbot logs
comment:8 Changed 11 years ago by
Dependencies:  → #10963 

Status:  needs_work → needs_review 
Replying to davidloeffler:
This fails doctests on every version tested  see patchbot logs
I forgot a dependency to #10963. Thanks for pointing this out.
comment:9 followup: 10 Changed 11 years ago by
Status:  needs_review → needs_info 

There is no patch at #10963. I don't think this can be meaningfully tagged as "needs review" when one of its dependencies doesn't exist yet.
comment:10 Changed 11 years ago by
Replying to davidloeffler:
There is no patch at #10963. I don't think this can be meaningfully tagged as "needs review" when one of its dependencies doesn't exist yet.
As you can see on #10963, the patch is far for being inexistent as Simon and Christian started to review it. Of course it is on the SageCombinat queue. I need to ping Nicolas to have him finish it. Now, I don't see why having Nicolas's patch unfinished forbid me to have some feedback on this one. And asking for such a feedback seems to be a perfectly meaningful reason for putting "needs review"... The only reason I can see for not doing it is that it can trouble the patchbot. Do you have another one ?
comment:11 Changed 11 years ago by
Salut Florent!
Why is there a dependency on 10963? In the queue, this patch is much lower than 10963.
comment:12 Changed 11 years ago by
Ah, I see, it's about the glitch Sets().Facade() <> Sets().Facades().
Since anyway the patch is up the queue, I suggest we finish commuting the two patches. Just use Sets().Facades() (we will need to keep the backward compatibility anyway).
With that, the tests pass up to to trivially failing doctests:
File "/opt/sage5.0.beta11/devel/sagecombinat/sage/structureset_factories.py", line 644: sage: P2.category() Expected: Join of Category of facade finite enumerated sets and Policy category with facade parent AllPairs Got: Join of Category of finite enumerated sets and Policy category with facade parent AllPairs ********************************************************************** File "/opt/sage5.0.beta11/devel/sagecombinat/sage/structure/set_factories.py", line 972: sage: P.category() Expected: Join of Category of facade finite enumerated sets and Policy category with facade parent AllPairs Got: Join of Category of finite enumerated sets and Policy category with facade parent AllPairs
Cheers,
comment:13 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

Changed 9 years ago by
Attachment:  trac_10194factories_policyfhv2.patch added 

comment:15 Changed 9 years ago by
Branch:  → u/chapoton/10194 

Commit:  → 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7 
comment:16 Changed 9 years ago by
Commit:  5dbd72b43c05ae78ddafb670ddc2cd31f06575a7 → 57f3ed3e2e888dffc5c1a860b8eb47662c7333e0 

Branch pushed to git repo; I updated commit sha1. New commits:
57f3ed3  trac #10194 minor changes

comment:17 Changed 9 years ago by
Commit:  57f3ed3e2e888dffc5c1a860b8eb47662c7333e0 → 28c42cb58e478741a0bad333b853c7f2aea86468 

Branch pushed to git repo; I updated commit sha1. New commits:
28c42cb  trac #10194 typo corrections

comment:18 Changed 9 years ago by
Dependencies:  #10963 

Temporary : emptied dependency field (was #10963)
comment:19 Changed 9 years ago by
Commit:  28c42cb58e478741a0bad333b853c7f2aea86468 → 6fa45d8a4c031d612966f336fbfc7be992911b14 

Branch pushed to git repo; I updated commit sha1. New commits:
6fa45d8  trac #10194 other details, still not working :(

comment:20 Changed 9 years ago by
Status:  needs_info → needs_work 

This needs to be worked upon, as it is not working at all.
comment:22 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:23 Changed 9 years ago by
Branch:  u/chapoton/10194 → u/hivert/10194 

comment:24 Changed 9 years ago by
Commit:  6fa45d8a4c031d612966f336fbfc7be992911b14 → 06beedce649e34107358217e13ef69bd31752ac5 

Branch pushed to git repo; I updated commit sha1. New commits:
06beedc  Fixed passing parent to ElementWrapper

comment:25 Changed 9 years ago by
Keywords:  days57 added 

comment:26 Changed 9 years ago by
Commit:  06beedce649e34107358217e13ef69bd31752ac5 → d7eb4c1317c4bc5a0d33312879d2938cc54e5d25 

Branch pushed to git repo; I updated commit sha1. New commits:
d7eb4c1  Cleanup in constraints managment.

comment:27 Changed 9 years ago by
Commit:  d7eb4c1317c4bc5a0d33312879d2938cc54e5d25 → b615150f459ea3f520cdc190325e961d3ce9ad22 

Branch pushed to git repo; I updated commit sha1. New commits:
b615150  Fixed printing of _repr_ for TopMostParentPolicy

comment:28 Changed 9 years ago by
Status:  needs_work → needs_review 

comment:30 Changed 9 years ago by
comment:31 followup: 32 Changed 9 years ago by
Then why do you have that in the commits of this ticket ?
comment:32 Changed 9 years ago by
Replying to chapoton:
Then why do you have that in the commits of this ticket ?
I probably screwed things up with git ! I'm trying to fix the problem.
comment:34 Changed 9 years ago by
Branch:  u/hivert/10194 → u/hivert/10194rebased 

comment:35 Changed 9 years ago by
Commit:  b615150f459ea3f520cdc190325e961d3ce9ad22 → fc6981b5ac4cf52c86973288c900ac08f8511042 

Should be fixed now ! But I had to rebase the branch. So Nicolas, you will have to rebase yours at some point. Sorry !
New commits:
dc5dfc8  # Mon Apr 07 00:47:29 2014 +0200

e595280  Fixed passing parent to ElementWrapper

80b08c3  Cleanup in constraints managment.

fc6981b  Fixed printing of _repr_ for TopMostParentPolicy

comment:36 Changed 9 years ago by
Branch:  u/hivert/10194rebased → u/hivert/10194rebasedbeta7 

comment:37 Changed 9 years ago by
Branch:  u/hivert/10194rebasedbeta7 → u/nborie/10194rebasedbeta7 

comment:38 Changed 9 years ago by
Branch:  u/nborie/10194rebasedbeta7 → public/ticket/10194 

Commit:  fc6981b5ac4cf52c86973288c900ac08f8511042 → 0eeecd1c94859b2328a1487e4d5dae91b158e5cb 
I have made one more commit. Now all tests pass, and code is in the pep8 standard.
New commits:
77279f9  # Mon Apr 07 00:47:29 2014 +0200

272d17f  Fixed passing parent to ElementWrapper

1491bff  Cleanup in constraints managment.

41c137e  Fixed printing of _repr_ for TopMostParentPolicy

f76e0e6  Review : fonctionality, documentations, tests (very small things remains...)

1f3e02b  Merge branch 'u/nborie/10194rebasedbeta7' of trac.sagemath.org:sage into 10194

0eeecd1  trac #10194 make sure that all test pass (and pyflakes and pep8 compliant)

comment:39 Changed 9 years ago by
There remains a pyflakes warning thta needs to be taken care of :
src/sage/structure/set_factories.py:722: undefined name 'FacadeParentPolicyCategory'
comment:40 Changed 9 years ago by
Status:  needs_review → needs_work 

Work issues:  doctest failures → pyflakes warning 
comment:41 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:42 Changed 9 years ago by
Commit:  0eeecd1c94859b2328a1487e4d5dae91b158e5cb → 16ca51901cd8a266b3ae59a90425deedd270d0a6 

comment:43 Changed 9 years ago by
what about the pyflakes warning ? could you take care of that please ?
comment:44 Changed 9 years ago by
Commit:  16ca51901cd8a266b3ae59a90425deedd270d0a6 → 014d842b9917c3090619cc9ea6aac681dde5c0bb 

comment:45 Changed 8 years ago by
Commit:  014d842b9917c3090619cc9ea6aac681dde5c0bb → e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d 

comment:46 Changed 8 years ago by
Status:  needs_work → needs_review 

Work issues:  pyflakes warning 
comment:47 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:48 Changed 8 years ago by
At last, got a green report from the bot. Florent, do you think that this is ready ?
comment:49 Changed 8 years ago by
Reviewers:  → Frédéric Chapoton 

comment:50 Changed 8 years ago by
Commit:  e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d → 87a475cdfa8d9cdd4ca6d3c4d337fd71570f3004 

Branch pushed to git repo; I updated commit sha1. New commits:
87a475c  Merge branch 'public/ticket/10194' of ssh://trac.sagemath.org:22/sage into 10194

comment:51 Changed 8 years ago by
Commit:  87a475cdfa8d9cdd4ca6d3c4d337fd71570f3004 → 641c28adea6a0ff81aeab6cf4a928e8c45613d6d 

Branch pushed to git repo; I updated commit sha1. New commits:
641c28a  trac #10194 minor doc changes

comment:52 Changed 8 years ago by
Commit:  641c28adea6a0ff81aeab6cf4a928e8c45613d6d → ed7f340783a1850d309af930f2fab698295482b9 

comment:53 Changed 8 years ago by
@Florent : What is the meaning of the sentences
Put a nice warning _single_pair
and
Comment that and put link to documentation caveat....
in the example file ?
comment:54 Changed 8 years ago by
Commit:  ed7f340783a1850d309af930f2fab698295482b9 → 20f17b3db7974884ec741d310638511574044b96 

Branch pushed to git repo; I updated commit sha1. New commits:
20f17b3  Merge branch 'public/ticket/10194' of trac.sagemath.org:sage into 6.4.b6

comment:56 Changed 8 years ago by
Commit:  20f17b3db7974884ec741d310638511574044b96 → 52550e0f8014e4fd0c32365426c4de3ddb0a36e5 

Branch pushed to git repo; I updated commit sha1. New commits:
52550e0  Merge branch 'develop' into t/10194/public/ticket/10194

comment:58 Changed 8 years ago by
Commit:  52550e0f8014e4fd0c32365426c4de3ddb0a36e5 → ab1507c0dc9386b8619cd098c1710ab9bd3de50c 

Branch pushed to git repo; I updated commit sha1. New commits:
ab1507c  Rereading set factories

comment:59 Changed 8 years ago by
Replying to chapoton:
9 failing doctests, see patchbot report
Hi Frederic,
I should have fixed most of the problems (including comment 53). I still need a last pass of rereading before putting it as needs review.
Florent
comment:60 Changed 8 years ago by
Commit:  ab1507c0dc9386b8619cd098c1710ab9bd3de50c → 586248e263a81254fa5395676d6cd7a1906bba12 

Branch pushed to git repo; I updated commit sha1. New commits:
586248e  Various cleanup in the documentation

comment:61 Changed 8 years ago by
Status:  needs_info → needs_review 

This should be ready for review
Florent
comment:62 Changed 8 years ago by
Commit:  586248e263a81254fa5395676d6cd7a1906bba12 → 763f93c95ecf8e79ff48339491212a6417d2c4f8 

Branch pushed to git repo; I updated commit sha1. New commits:
763f93c  Merge tag '6.7' into t/10194/factories_policy

comment:63 followup: 64 Changed 8 years ago by
Milestone:  sage6.4 → sage6.8 

Was there any need for a merge ? By the way, I usually do the merge in the other direction. This gives much smaller commits, as you can see in the commit list.
comment:64 Changed 8 years ago by
Replying to chapoton:
Was there any need for a merge ? By the way, I usually do the merge in the other direction. This gives much smaller commits, as you can see in the commit list.
No there wasn't, but my goal is to update trees to use factories and ultimately to do computation in operads. As you merged 6.7 in operads I also merged this one to avoid going back and forth.
comment:65 Changed 7 years ago by
Description:  modified (diff) 

Status:  needs_review → positive_review 
ok, let this get in. There are several combinatorial tickets on top of it.
comment:66 Changed 7 years ago by
Description:  modified (diff) 

comment:67 Changed 7 years ago by
Branch:  public/ticket/10194 → 763f93c95ecf8e79ff48339491212a6417d2c4f8 

Resolution:  → fixed 
Status:  positive_review → closed 
Patch under development on the SageCombinat patch server.