#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 )
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.
Attachments (1)
Change History (28)
comment:1 Changed 6 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 6 years ago by
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 6 years ago by
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
Thanks Florent for your help (privately) with ClonableIntArray?. The new patch is attached.
Anne
comment:5 Changed 6 years ago by
- 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: ↓ 7 Changed 6 years ago by
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
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
- Reviewers changed from Nicolas M. Thiéry to Nicolas M. Thiéry, Anne Schilling
comment:9 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:10 Changed 6 years ago by
- Description modified (diff)
comment:11 follow-up: ↓ 12 Changed 6 years ago by
- Description modified (diff)
Please review!
Thanks,
Anne
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 6 years ago by
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: ↓ 15 Changed 6 years ago by
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 6 years ago by
- Status changed from needs_review to positive_review
comment:15 in reply to: ↑ 13 ; follow-up: ↓ 17 Changed 6 years ago by
- 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 6 years ago by
- Status changed from needs_work to needs_review
comment:17 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 6 years ago by
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 6 years ago by
- Status changed from needs_review to positive_review
comment:19 in reply to: ↑ 17 ; follow-up: ↓ 20 Changed 6 years ago by
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 6 years ago by
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 21 Changed 6 years ago by
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 6 years ago by
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: ↓ 23 Changed 6 years ago by
- Milestone changed from sage-5.0 to sage-5.1
comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 6 years ago by
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: ↓ 25 Changed 6 years ago by
- 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 6 years ago by
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: ↓ 27 Changed 6 years ago by
- Merged in set to sage-5.0.beta14
- Resolution set to fixed
- Status changed from positive_review to closed
Hi Anne,
There are some problems with the doc:
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.