Opened 7 years ago

Closed 2 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) Commit: 763f93c95ecf8e79ff48339491212a6417d2c4f8
Dependencies: Stopgaps:

Description (last modified by chapoton)

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 constraints cons. 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)

trac_10194-factories_policy-fh.patch (57.1 KB) - added by hivert 6 years ago.
trac_10194-factories_policy-fh-v2.patch (57.2 KB) - added by chapoton 4 years ago.

Download all attachments as: .zip

Change History (69)

comment:1 Changed 7 years ago by nthiery

Patch under development on the Sage-Combinat patch server.

comment:2 Changed 7 years ago by hivert

  • Description modified (diff)
  • Owner changed from sage-combinat to hivert
  • Summary changed from Enumerated set factories to Set factories

comment:3 Changed 7 years ago by hivert

  • Keywords factories days30 added

comment:4 Changed 6 years ago by hivert

  • Keywords Cernay2012 added

comment:5 Changed 6 years ago by hivert

  • Milestone set to sage-5.0

comment:6 Changed 6 years ago by hivert

  • Description modified (diff)
  • Status changed from new to needs_review

Changed 6 years ago by hivert

comment:7 follow-up: Changed 6 years ago by davidloeffler

  • 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 hivert

  • 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: Changed 6 years ago by davidloeffler

  • 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 hivert

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 6 years ago by nthiery

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 nthiery

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 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 4 years ago by chapoton

comment:14 Changed 4 years ago by chapoton

new version, rebased on 5.12.beta0, with cleanup

comment:15 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/10194
  • Commit set to 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7

I have made a git branch, but it seems to be massively broken.

Some problems come from Sets().Facade() versus Sets().Facades() as explained before

Some other problems come from #14519

And some other problems I do not understand..


New commits:

5dbd72btrac_10194-factories_policy-fh.patch

comment:16 Changed 4 years ago by git

  • Commit changed from 5dbd72b43c05ae78ddafb670ddc2cd31f06575a7 to 57f3ed3e2e888dffc5c1a860b8eb47662c7333e0

Branch pushed to git repo; I updated commit sha1. New commits:

57f3ed3trac #10194 minor changes

comment:17 Changed 4 years ago by git

  • Commit changed from 57f3ed3e2e888dffc5c1a860b8eb47662c7333e0 to 28c42cb58e478741a0bad333b853c7f2aea86468

Branch pushed to git repo; I updated commit sha1. New commits:

28c42cbtrac #10194 typo corrections

comment:18 Changed 4 years ago by chapoton

  • Dependencies #10963 deleted

Temporary : emptied dependency field (was #10963)

comment:19 Changed 4 years ago by git

  • Commit changed from 28c42cb58e478741a0bad333b853c7f2aea86468 to 6fa45d8a4c031d612966f336fbfc7be992911b14

Branch pushed to git repo; I updated commit sha1. New commits:

6fa45d8trac #10194 other details, still not working :(

comment:20 Changed 4 years ago by chapoton

  • 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 chapoton

Maybe some of the problems come from #13801 ?

comment:22 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:23 Changed 4 years ago by hivert

  • Branch changed from u/chapoton/10194 to u/hivert/10194

comment:24 Changed 4 years ago by git

  • Commit changed from 6fa45d8a4c031d612966f336fbfc7be992911b14 to 06beedce649e34107358217e13ef69bd31752ac5

Branch pushed to git repo; I updated commit sha1. New commits:

06beedcFixed passing parent to ElementWrapper

comment:25 Changed 4 years ago by nborie

  • Keywords days57 added

comment:26 Changed 4 years ago by git

  • Commit changed from 06beedce649e34107358217e13ef69bd31752ac5 to d7eb4c1317c4bc5a0d33312879d2938cc54e5d25

Branch pushed to git repo; I updated commit sha1. New commits:

d7eb4c1Cleanup in constraints managment.

comment:27 Changed 4 years ago by git

  • Commit changed from d7eb4c1317c4bc5a0d33312879d2938cc54e5d25 to b615150f459ea3f520cdc190325e961d3ce9ad22

Branch pushed to git repo; I updated commit sha1. New commits:

b615150Fixed printing of _repr_ for TopMostParentPolicy

comment:28 Changed 4 years ago by hivert

  • Status changed from needs_work to needs_review

comment:29 follow-up: Changed 4 years ago by chapoton

Maybe you should add #13580 as a dependency ?

comment:30 in reply to: ↑ 29 Changed 4 years ago by hivert

Replying to chapoton:

Maybe you should add #13580 as a dependency ?

Why ? Those are completely different things.

comment:31 follow-up: Changed 4 years ago by chapoton

Then why do you have that in the commits of this ticket ?

comment:32 in reply to: ↑ 31 Changed 4 years ago by hivert

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 nborie

For information, this features is largely tested in #16094

comment:34 Changed 4 years ago by hivert

  • Branch changed from u/hivert/10194 to u/hivert/10194-rebased

comment:35 Changed 4 years ago by hivert

  • 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
e595280Fixed passing parent to ElementWrapper
80b08c3Cleanup in constraints managment.
fc6981bFixed printing of _repr_ for TopMostParentPolicy

comment:36 Changed 4 years ago by hivert

  • Branch changed from u/hivert/10194-rebased to u/hivert/10194-rebased-beta7

comment:37 Changed 4 years ago by nborie

  • Branch changed from u/hivert/10194-rebased-beta7 to u/nborie/10194-rebased-beta7

comment:38 Changed 4 years ago by chapoton

  • 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
272d17fFixed passing parent to ElementWrapper
1491bffCleanup in constraints managment.
41c137eFixed printing of _repr_ for TopMostParentPolicy
f76e0e6Review : fonctionality, documentations, tests (very small things remains...)
1f3e02bMerge branch 'u/nborie/10194-rebased-beta7' of trac.sagemath.org:sage into 10194
0eeecd1trac #10194 make sure that all test pass (and pyflakes and pep8 compliant)

comment:39 Changed 4 years ago by chapoton

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 chapoton

  • Status changed from needs_review to needs_work
  • Work issues changed from doctest failures to pyflakes warning

comment:41 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:42 Changed 4 years ago by git

  • Commit changed from 0eeecd1c94859b2328a1487e4d5dae91b158e5cb to 16ca51901cd8a266b3ae59a90425deedd270d0a6

Branch pushed to git repo; I updated commit sha1. New commits:

f823667Minor doc improvements
16ca519Merge branch 'public/ticket/10194' of trac.sagemath.org:sage into t/10194/factories_policy

comment:43 Changed 4 years ago by chapoton

what about the pyflakes warning ? could you take care of that please ?

comment:44 Changed 3 years ago by git

  • Commit changed from 16ca51901cd8a266b3ae59a90425deedd270d0a6 to 014d842b9917c3090619cc9ea6aac681dde5c0bb

Branch pushed to git repo; I updated commit sha1. New commits:

364a0c0Merge branch 'public/ticket/10194' of ssh://trac.sagemath.org:22/sage into 10194
014d842trac #10194 details and formatting

comment:45 Changed 3 years ago by git

  • Commit changed from 014d842b9917c3090619cc9ea6aac681dde5c0bb to e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d

Branch pushed to git repo; I updated commit sha1. New commits:

4895320Merge branch 'public/ticket/10194' of ssh://trac.sagemath.org:22/sage into 10194
e061484trac #10194 removed the category method (was useless and wrong ?)

comment:46 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review
  • Work issues pyflakes warning deleted

comment:47 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:48 Changed 3 years ago by chapoton

At last, got a green report from the bot. Florent, do you think that this is ready ?

comment:49 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton

comment:50 Changed 3 years ago by git

  • Commit changed from e061484adfe23b2b8c2a5d0a1fafe4cff1de2b5d to 87a475cdfa8d9cdd4ca6d3c4d337fd71570f3004

Branch pushed to git repo; I updated commit sha1. New commits:

87a475cMerge branch 'public/ticket/10194' of ssh://trac.sagemath.org:22/sage into 10194

comment:51 Changed 3 years ago by git

  • Commit changed from 87a475cdfa8d9cdd4ca6d3c4d337fd71570f3004 to 641c28adea6a0ff81aeab6cf4a928e8c45613d6d

Branch pushed to git repo; I updated commit sha1. New commits:

641c28atrac #10194 minor doc changes

comment:52 Changed 3 years ago by git

  • Commit changed from 641c28adea6a0ff81aeab6cf4a928e8c45613d6d to ed7f340783a1850d309af930f2fab698295482b9

Branch pushed to git repo; I updated commit sha1. New commits:

3c12f5fMerge with 6.3.beta4
ed7f340trac #10194 found a few typo again

comment:53 Changed 3 years ago by chapoton

@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 git

  • Commit changed from ed7f340783a1850d309af930f2fab698295482b9 to 20f17b3db7974884ec741d310638511574044b96

Branch pushed to git repo; I updated commit sha1. New commits:

20f17b3Merge branch 'public/ticket/10194' of trac.sagemath.org:sage into 6.4.b6

comment:55 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_info

@Florent : PING, cf comment 53

comment:56 Changed 3 years ago by git

  • Commit changed from 20f17b3db7974884ec741d310638511574044b96 to 52550e0f8014e4fd0c32365426c4de3ddb0a36e5

Branch pushed to git repo; I updated commit sha1. New commits:

52550e0Merge branch 'develop' into t/10194/public/ticket/10194

comment:57 follow-up: Changed 3 years ago by chapoton

9 failing doctests, see patchbot report

comment:58 Changed 3 years ago by git

  • Commit changed from 52550e0f8014e4fd0c32365426c4de3ddb0a36e5 to ab1507c0dc9386b8619cd098c1710ab9bd3de50c

Branch pushed to git repo; I updated commit sha1. New commits:

ab1507cRe-reading set factories

comment:59 in reply to: ↑ 57 Changed 3 years ago by hivert

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 3 years ago by git

  • Commit changed from ab1507c0dc9386b8619cd098c1710ab9bd3de50c to 586248e263a81254fa5395676d6cd7a1906bba12

Branch pushed to git repo; I updated commit sha1. New commits:

586248eVarious cleanup in the documentation

comment:61 Changed 3 years ago by hivert

  • Status changed from needs_info to needs_review

This should be ready for review

Florent

comment:62 Changed 3 years ago by git

  • Commit changed from 586248e263a81254fa5395676d6cd7a1906bba12 to 763f93c95ecf8e79ff48339491212a6417d2c4f8

Branch pushed to git repo; I updated commit sha1. New commits:

763f93cMerge tag '6.7' into t/10194/factories_policy

comment:63 follow-up: Changed 3 years ago by chapoton

  • 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.

Last edited 3 years ago by chapoton (previous) (diff)

comment:64 in reply to: ↑ 63 Changed 3 years ago by hivert

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.

Last edited 3 years ago by hivert (previous) (diff)

comment:65 Changed 2 years ago by chapoton

  • 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 chapoton

  • Description modified (diff)

comment:67 Changed 2 years ago by vbraun

  • Branch changed from public/ticket/10194 to 763f93c95ecf8e79ff48339491212a6417d2c4f8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.