Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#12536 closed enhancement (fixed)

Implementation of class for Linear Extensions of a finite Poset

Reported by: aschilling Owned by: sage-combinat
Priority: major Milestone: sage-5.0
Component: combinatorics Keywords: posets, linear extensions
Cc: sage-combinat Merged in: sage-5.0.beta14
Authors: Anne Schilling, Nicolas M. Thiéry Reviewers: Nicolas M. Thiéry, Anne Schilling
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12677 Stopgaps:

Description (last modified by aschilling)

This patch implements a class for linear extensions of a finite poset. It also add functionalities to the poset class, such as the promotion and evacuation operators.

Apply: trac_12536-linear_extensions-as.patch

Attachments (1)

trac_12536-linear_extensions-as.patch (56.1 KB) - added by aschilling 5 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 6 years ago by aschilling

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

comment:2 follow-up: Changed 6 years ago by hivert

Hi Anne,

There are some problems with the doc:

/home/data/Sage-Install/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/combinat/posets/posets.py:docstring of sage.combinat.posets.posets.FinitePoset.promotion:10: WARNING: Inline literal start-string without end-string.
/home/data/Sage-Install/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/combinat/posets/posets.py:docstring of sage.combinat.posets.posets.FinitePoset.promotion:23: WARNING: Duplicate explicit target name: "stanley2009".
/home/data/Sage-Install/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/combinat/posets/posets.py:docstring of sage.combinat.posets.posets.FinitePoset.promotion:23: WARNING: duplicate citation Stanley2009, other instance in /home/data/Sage-Install/sage-5.0.beta2/devel/sage/doc/en/reference/sage/combinat/posets/posets.rst

Note: if you cite the same paper in two different function, you should either put the reference at the beginning of the file and link to it, or put two different refs. I usually use the second methods which is not very satisfactory.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 6 years ago by aschilling

Hi Florent,

Thanks for looking at this. This should be fixed now in the new version of the patch. Also, /combinat/posets/linear_extensions should now appear in the documentation.

If you have comments on how to use ClonableIntArray?, let me know!

Anne

comment:4 in reply to: ↑ 3 Changed 6 years ago by aschilling

Thanks Florent for your help (privately) with ClonableIntArray?. The new patch is attached.

Anne

comment:5 Changed 6 years ago by nthiery

  • Dependencies set to #12677
  • Description modified (diff)
  • Reviewers set to Nicolas M. Thiéry
  • Status changed from needs_review to needs_work

comment:6 follow-up: Changed 6 years ago by nthiery

There were two failing doctests due to dependence of sageinspect's doctest on Poset. This dependence is removed in #12677.

comment:7 in reply to: ↑ 6 Changed 6 years ago by aschilling

Hi Nicolas,

Thanks for your review patch. I posted a new reviewer's patch on the sage-combinat queue. If you are satisfied I can fold everything and post it here.

Thanks,

Anne

comment:8 Changed 6 years ago by aschilling

  • Authors changed from Anne Schilling to Anne Schilling, Nicolas M. Thiéry
  • Reviewers changed from Nicolas M. Thiéry to Nicolas M. Thiéry, Anne Schilling

comment:9 Changed 6 years ago by aschilling

  • Status changed from needs_work to needs_review

comment:10 Changed 6 years ago by aschilling

  • Description modified (diff)

comment:11 follow-up: Changed 6 years ago by aschilling

  • Description modified (diff)

Please review!

Thanks,

Anne

comment:12 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by aschilling

Hi Nicolas,

Any possibility to review this very soon to get this into sage-5.0? I would like to have this for Sage Days 38.

Thank you,

Anne

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by aschilling

Comment by Nicolas:

"If this suits you, please do the similar change in the evacuation method, and add some example(s) with a Poset having arbitrary labels, and that's good to go!"

I did those changes and fixed the last doc string failures.

So I am setting a positive review.

Anne

comment:14 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:15 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by nthiery

  • Status changed from positive_review to needs_work

Replying to aschilling:

Comment by Nicolas:

"If this suits you, please do the similar change in the evacuation method, and add some example(s) with a Poset having arbitrary labels, and that's good to go!"

I did those changes and fixed the last doc string failures.

Well, this comment was not exactly about *those* changes. But I am indeed very happy with that final version, so ok for the positive review! Thanks for your further work on that patch.

You will find on the sage-combinat patch server a reviewers patch with some final suggestions of documentation improvements (beside some minor details; it felt useful to pinpoint that the result of evacuation and promotion was a poset). Fold in whatever you like, throw away the rest, and set back a positive review on my behalf.

Cheers,

Nicolas

comment:16 Changed 5 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:17 in reply to: ↑ 15 ; follow-up: Changed 5 years ago by aschilling

You will find on the sage-combinat patch server a reviewers patch with some final suggestions of documentation improvements (beside some minor details; it felt useful to pinpoint that the result of evacuation and promotion was a poset). Fold in whatever you like, throw away the rest, and set back a positive review on my behalf.

Thank you for the reviewer's patch. I folded it and uploaded the new version and set it back to positive review on your behalf.

Cheers,

Anne

comment:18 Changed 5 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:19 in reply to: ↑ 17 ; follow-up: Changed 5 years ago by nthiery

Replying to aschilling:

I folded it and uploaded the new version and set it back to positive review on your behalf.

It's going to be a cool addition to Sage. Thanks for your hard work on that patch, and looking forward your talk at Sage Days 38 (and discussions on the topic before that)!

Cheers,

Nicolas

Changed 5 years ago by aschilling

comment:20 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by aschilling

I just moved one line in the code for efficiency. Tests still pass. Would it be possible to merge this patch soon for Sage Days 38?

Thank you,

Anne

comment:21 in reply to: ↑ 20 Changed 5 years ago by nthiery

Replying to aschilling:

I just moved one line in the code for efficiency. Tests still pass. Would it be possible to merge this patch soon for Sage Days 38?

For the record: I validate the change, and the relevant tests passed on my machine. The change is trivial enough that I see no reason why it could possibly break on other platforms.

comment:22 follow-up: Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-5.1

comment:23 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by nthiery

Hi Jeroen,

Replying to jdemeyer:

Milestone changed from sage-5.0 to sage-5.1

We understand that the two following requests are quite antagonist, yet we would very much appreciate if this patch (which is fairly harmless) could be merged in 5.0, and 5.0 could be released soon. Indeed, the implemented feature will be one of the themes of Sage Days 38 (Anne will give a talk about them); so it would be practical to have them in vanilla Sage.

If a choice has to be done between the two requests, then our preference is for 5.0 to be released soon.

Thanks for your hard work!

Nicolas

comment:24 in reply to: ↑ 23 ; follow-up: Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.0

Replying to nthiery:

we would very much appreciate if this patch (which is fairly harmless) could be merged in 5.0.

I don't think such a big patch can ever be harmless, but I'll try it.

If a choice has to be done between the two requests

Certainly not, as this ticket is not likely to influence the release date of sage-5.0.

To be totally honest, I wouldn't count on sage-5.0 to be released by Sage Days 38. It could be, but then we quickly need some progress with the blocker tickets.

comment:25 in reply to: ↑ 24 Changed 5 years ago by nthiery

Replying to jdemeyer:

I don't think such a big patch can ever be harmless, but I'll try it.

Thanks, I appreciate that!

(by harmless, I mean't that it sure could be broken by itself, but by nature it's unlikely to have any influence any other parts of Sage, and thus break something preexisting without it getting detected by the testsuite)

To be totally honest, I wouldn't count on sage-5.0 to be released by Sage Days 38. It could be, but then we quickly need some progress with the blocker tickets.

Ok. At least one should be fixed soon: Florent just told me he found why the argspec had disapeared in the sphinx-compiled documentation :-)

Cheers,

Nicolas

comment:26 follow-up: Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 in reply to: ↑ 26 Changed 5 years ago by aschilling

Replying to jdemeyer:

Thank you for merging the patch!!

Anne

Note: See TracTickets for help on using tickets.