Opened 7 years ago
Closed 2 years ago
#10194 closed enhancement (fixed)
Set factories
Reported by:  nthiery  Owned by:  hivert 

Priority:  major  Milestone:  sage6.8 
Component:  combinatorics  Keywords:  factories, days30, Cernay2012, days57 
Cc:  sagecombinat, hivert  Merged in:  
Authors:  Florent Hivert  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  763f93c (Commits)  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 7 years ago by
comment:2 Changed 7 years ago by
 Description modified (diff)
 Owner changed from sagecombinat to hivert
 Summary changed from Enumerated set factories to Set factories
comment:3 Changed 7 years ago by
 Keywords factories days30 added
comment:4 Changed 6 years ago by
 Keywords Cernay2012 added
comment:5 Changed 6 years ago by
 Milestone set to sage5.0
comment:6 Changed 6 years ago by
 Description modified (diff)
 Status changed from new to needs_review
Changed 6 years ago by
comment:7 followup: ↓ 8 Changed 6 years ago by
 Status changed from needs_review to needs_work
 Work issues set to doctest failures
This fails doctests on every version tested  see patchbot logs
comment:8 in reply to: ↑ 7 Changed 6 years ago by
 Dependencies set to #10963
 Status changed from needs_work to 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 6 years ago by
 Status changed from needs_review to 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 in reply to: ↑ 9 Changed 6 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 6 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 6 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 4 years ago by
 Milestone changed from sage5.11 to sage5.12
Changed 4 years ago by
comment:14 Changed 4 years ago by
new version, rebased on 5.12.beta0, with cleanup
comment:15 Changed 4 years ago by
 Branch set to u/chapoton/10194
 Commit set to 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7
comment:16 Changed 4 years ago by
 Commit changed from 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7 to 57f3ed3e2e888dffc5c1a860b8eb47662c7333e0
Branch pushed to git repo; I updated commit sha1. New commits:
57f3ed3  trac #10194 minor changes

comment:17 Changed 4 years ago by
 Commit changed from 57f3ed3e2e888dffc5c1a860b8eb47662c7333e0 to 28c42cb58e478741a0bad333b853c7f2aea86468
Branch pushed to git repo; I updated commit sha1. New commits:
28c42cb  trac #10194 typo corrections

comment:18 Changed 4 years ago by
 Dependencies #10963 deleted
Temporary : emptied dependency field (was #10963)
comment:19 Changed 4 years ago by
 Commit changed from 28c42cb58e478741a0bad333b853c7f2aea86468 to 6fa45d8a4c031d612966f336fbfc7be992911b14
Branch pushed to git repo; I updated commit sha1. New commits:
6fa45d8  trac #10194 other details, still not working :(

comment:20 Changed 4 years ago by
 Status changed from needs_info to needs_work
This needs to be worked upon, as it is not working at all.
comment:21 Changed 4 years ago by
Maybe some of the problems come from #13801 ?
comment:22 Changed 4 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:23 Changed 4 years ago by
 Branch changed from u/chapoton/10194 to u/hivert/10194
comment:24 Changed 4 years ago by
 Commit changed from 6fa45d8a4c031d612966f336fbfc7be992911b14 to 06beedce649e34107358217e13ef69bd31752ac5
Branch pushed to git repo; I updated commit sha1. New commits:
06beedc  Fixed passing parent to ElementWrapper

comment:25 Changed 4 years ago by
 Keywords days57 added
comment:26 Changed 4 years ago by
 Commit changed from 06beedce649e34107358217e13ef69bd31752ac5 to d7eb4c1317c4bc5a0d33312879d2938cc54e5d25
Branch pushed to git repo; I updated commit sha1. New commits:
d7eb4c1  Cleanup in constraints managment.

comment:27 Changed 4 years ago by
 Commit changed from d7eb4c1317c4bc5a0d33312879d2938cc54e5d25 to b615150f459ea3f520cdc190325e961d3ce9ad22
Branch pushed to git repo; I updated commit sha1. New commits:
b615150  Fixed printing of _repr_ for TopMostParentPolicy

comment:28 Changed 4 years ago by
 Status changed from needs_work to needs_review
comment:29 followup: ↓ 30 Changed 4 years ago by
Maybe you should add #13580 as a dependency ?
comment:30 in reply to: ↑ 29 Changed 4 years ago by
comment:31 followup: ↓ 32 Changed 4 years ago by
Then why do you have that in the commits of this ticket ?
comment:32 in reply to: ↑ 31 Changed 4 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:33 Changed 4 years ago by
For information, this features is largely tested in #16094
comment:34 Changed 4 years ago by
 Branch changed from u/hivert/10194 to u/hivert/10194rebased
comment:35 Changed 4 years ago by
 Commit changed from b615150f459ea3f520cdc190325e961d3ce9ad22 to 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 4 years ago by
 Branch changed from u/hivert/10194rebased to u/hivert/10194rebasedbeta7
comment:37 Changed 4 years ago by
 Branch changed from u/hivert/10194rebasedbeta7 to u/nborie/10194rebasedbeta7
comment:38 Changed 4 years ago by
 Branch changed from u/nborie/10194rebasedbeta7 to public/ticket/10194
 Commit changed from fc6981b5ac4cf52c86973288c900ac08f8511042 to 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 4 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 4 years ago by
 Status changed from needs_review to needs_work
 Work issues changed from doctest failures to pyflakes warning
comment:41 Changed 4 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:42 Changed 4 years ago by
 Commit changed from 0eeecd1c94859b2328a1487e4d5dae91b158e5cb to 16ca51901cd8a266b3ae59a90425deedd270d0a6
comment:43 Changed 4 years ago by
what about the pyflakes warning ? could you take care of that please ?
comment:44 Changed 4 years ago by
 Commit changed from 16ca51901cd8a266b3ae59a90425deedd270d0a6 to 014d842b9917c3090619cc9ea6aac681dde5c0bb
comment:45 Changed 4 years ago by
 Commit changed from 014d842b9917c3090619cc9ea6aac681dde5c0bb to e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d
comment:46 Changed 4 years ago by
 Status changed from needs_work to needs_review
 Work issues pyflakes warning deleted
comment:47 Changed 3 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:48 Changed 3 years ago by
At last, got a green report from the bot. Florent, do you think that this is ready ?
comment:49 Changed 3 years ago by
 Reviewers set to Frédéric Chapoton
comment:50 Changed 3 years ago by
 Commit changed from e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d to 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 3 years ago by
 Commit changed from 87a475cdfa8d9cdd4ca6d3c4d337fd71570f3004 to 641c28adea6a0ff81aeab6cf4a928e8c45613d6d
Branch pushed to git repo; I updated commit sha1. New commits:
641c28a  trac #10194 minor doc changes

comment:52 Changed 3 years ago by
 Commit changed from 641c28adea6a0ff81aeab6cf4a928e8c45613d6d to ed7f340783a1850d309af930f2fab698295482b9
comment:53 Changed 3 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 3 years ago by
 Commit changed from ed7f340783a1850d309af930f2fab698295482b9 to 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:55 Changed 3 years ago by
 Status changed from needs_review to needs_info
@Florent : PING, cf comment 53
comment:56 Changed 3 years ago by
 Commit changed from 20f17b3db7974884ec741d310638511574044b96 to 52550e0f8014e4fd0c32365426c4de3ddb0a36e5
Branch pushed to git repo; I updated commit sha1. New commits:
52550e0  Merge branch 'develop' into t/10194/public/ticket/10194

comment:57 followup: ↓ 59 Changed 3 years ago by
9 failing doctests, see patchbot report
comment:58 Changed 3 years ago by
 Commit changed from 52550e0f8014e4fd0c32365426c4de3ddb0a36e5 to ab1507c0dc9386b8619cd098c1710ab9bd3de50c
Branch pushed to git repo; I updated commit sha1. New commits:
ab1507c  Rereading set factories

comment:59 in reply to: ↑ 57 Changed 3 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 3 years ago by
 Commit changed from ab1507c0dc9386b8619cd098c1710ab9bd3de50c to 586248e263a81254fa5395676d6cd7a1906bba12
Branch pushed to git repo; I updated commit sha1. New commits:
586248e  Various cleanup in the documentation

comment:61 Changed 3 years ago by
 Status changed from needs_info to needs_review
This should be ready for review
Florent
comment:62 Changed 3 years ago by
 Commit changed from 586248e263a81254fa5395676d6cd7a1906bba12 to 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 3 years ago by
 Milestone changed from sage6.4 to 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 in reply to: ↑ 63 Changed 3 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.
There is not diff before and after the merge so that you can review on the unmerged version.
comment:65 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
ok, let this get in. There are several combinatorial tickets on top of it.
comment:66 Changed 2 years ago by
 Description modified (diff)
comment:67 Changed 2 years ago by
 Branch changed from public/ticket/10194 to 763f93c95ecf8e79ff48339491212a6417d2c4f8
 Resolution set to fixed
 Status changed from positive_review to closed
Patch under development on the SageCombinat patch server.