Opened 12 years ago
Closed 7 years ago
#10194 closed enhancement (fixed)
Set factories
Reported by: | nthiery | Owned by: | hivert |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | combinatorics | Keywords: | factories, days30, Cernay2012, days57 |
Cc: | sage-combinat, 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 11 years ago by
- Description modified (diff)
- Owner changed from sage-combinat to hivert
- Summary changed from Enumerated set factories to Set factories
comment:3 Changed 11 years ago by
- Keywords factories days30 added
comment:4 Changed 10 years ago by
- Keywords Cernay2012 added
comment:5 Changed 10 years ago by
- Milestone set to sage-5.0
comment:6 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
Changed 10 years ago by
comment:7 follow-up: ↓ 8 Changed 10 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 10 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 follow-up: ↓ 10 Changed 10 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 10 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 Sage-Combinat 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 10 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 10 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/sage-5.0.beta11/devel/sage-combinat/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/sage-5.0.beta11/devel/sage-combinat/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 changed from sage-5.11 to sage-5.12
Changed 9 years ago by
comment:14 Changed 9 years ago by
new version, rebased on 5.12.beta0, with cleanup
comment:15 Changed 8 years ago by
- Branch set to u/chapoton/10194
- Commit set to 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7
comment:16 Changed 8 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 8 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 8 years ago by
- Dependencies #10963 deleted
Temporary : emptied dependency field (was #10963)
comment:19 Changed 8 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 8 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 8 years ago by
Maybe some of the problems come from #13801 ?
comment:22 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:23 Changed 8 years ago by
- Branch changed from u/chapoton/10194 to u/hivert/10194
comment:24 Changed 8 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 8 years ago by
- Keywords days57 added
comment:26 Changed 8 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 8 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 8 years ago by
- Status changed from needs_work to needs_review
comment:29 follow-up: ↓ 30 Changed 8 years ago by
Maybe you should add #13580 as a dependency ?
comment:30 in reply to: ↑ 29 Changed 8 years ago by
comment:31 follow-up: ↓ 32 Changed 8 years ago by
Then why do you have that in the commits of this ticket ?
comment:32 in reply to: ↑ 31 Changed 8 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 8 years ago by
For information, this features is largely tested in #16094
comment:34 Changed 8 years ago by
- Branch changed from u/hivert/10194 to u/hivert/10194-rebased
comment:35 Changed 8 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 8 years ago by
- Branch changed from u/hivert/10194-rebased to u/hivert/10194-rebased-beta7
comment:37 Changed 8 years ago by
- Branch changed from u/hivert/10194-rebased-beta7 to u/nborie/10194-rebased-beta7
comment:38 Changed 8 years ago by
- Branch changed from u/nborie/10194-rebased-beta7 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/10194-rebased-beta7' of trac.sagemath.org:sage into 10194
|
0eeecd1 | trac #10194 make sure that all test pass (and pyflakes and pep8 compliant)
|
comment:39 Changed 8 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 8 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from doctest failures to pyflakes warning
comment:41 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:42 Changed 8 years ago by
- Commit changed from 0eeecd1c94859b2328a1487e4d5dae91b158e5cb to 16ca51901cd8a266b3ae59a90425deedd270d0a6
comment:43 Changed 8 years ago by
what about the pyflakes warning ? could you take care of that please ?
comment:44 Changed 8 years ago by
- Commit changed from 16ca51901cd8a266b3ae59a90425deedd270d0a6 to 014d842b9917c3090619cc9ea6aac681dde5c0bb
comment:45 Changed 8 years ago by
- Commit changed from 014d842b9917c3090619cc9ea6aac681dde5c0bb to e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d
comment:46 Changed 8 years ago by
- Status changed from needs_work to needs_review
- Work issues pyflakes warning deleted
comment:47 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.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 set to Frédéric Chapoton
comment:50 Changed 8 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 8 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 8 years ago by
- Commit changed from 641c28adea6a0ff81aeab6cf4a928e8c45613d6d to 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 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 8 years ago by
- Status changed from needs_review to needs_info
@Florent : PING, cf comment 53
comment:56 Changed 7 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 follow-up: ↓ 59 Changed 7 years ago by
9 failing doctests, see patchbot report
comment:58 Changed 7 years ago by
- Commit changed from 52550e0f8014e4fd0c32365426c4de3ddb0a36e5 to ab1507c0dc9386b8619cd098c1710ab9bd3de50c
Branch pushed to git repo; I updated commit sha1. New commits:
ab1507c | Re-reading set factories
|
comment:59 in reply to: ↑ 57 Changed 7 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 re-reading before putting it as needs review.
Florent
comment:60 Changed 7 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 7 years ago by
- Status changed from needs_info to needs_review
This should be ready for review
Florent
comment:62 Changed 7 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 follow-up: ↓ 64 Changed 7 years ago by
- Milestone changed from sage-6.4 to sage-6.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 7 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 7 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 7 years ago by
- Description modified (diff)
comment:67 Changed 7 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 Sage-Combinat patch server.