Ticket #7157 (closed enhancement: fixed)
neighbors_out/in instead of predecessor/successor in DiGraph
| Reported by: | ncohen | Owned by: | rlm |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.3 |
| Component: | graph theory | Keywords: | |
| Cc: | jason, rlm, nthiery | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Florent Hivert, Mike Hansen |
| Authors: | Nathann Cohen | Merged in: | sage-4.3.alpha1 |
| Dependencies: | Stopgaps: |
Attachments
-
trac_7157.patch
(10.3 KB) -
added by ncohen 3 years ago.
-
trac_7157_review.patch
(2.6 KB) -
added by mhansen 3 years ago.
Change History
comment:2 Changed 4 years ago by rlm
- Status changed from needs_review to needs_work
- Summary changed from [with patch, needs review] neighbors_out/in instead of predecessor/successor in DiGraph to [with patch, needs work] neighbors_out/in instead of predecessor/successor in DiGraph
You shouldn't just remove the successor/predecessor terminology. A lot of people (e.g. Chris Godsil) might have to do a lot of work to change all their personal scripts etc. to reflect this change. This patch breaks backwards compatibility.
comment:3 follow-up: ↓ 4 Changed 4 years ago by ncohen
Should I just add aliases and let the old functions exist ? Should we keep two copies of the same functions ?
comment:4 in reply to: ↑ 3 Changed 4 years ago by rlm
Replying to ncohen:
Should I just add aliases and let the old functions exist ? Should we keep two copies of the same functions ?
I think aliases would be okay, but people have mentioned before that aliases are bad. Please bring this up on the sage-devel thread, and do what the consensus is there.
comment:5 Changed 4 years ago by ncohen
- Status changed from needs_work to needs_review
- Summary changed from [with patch, needs work] neighbors_out/in instead of predecessor/successor in DiGraph to [with patch, needs review] neighbors_out/in instead of predecessor/successor in DiGraph
Should be better now :-)
The new functions are defined, old functions are marked "Deprecated".
comment:6 Changed 3 years ago by hivert
- Report Upstream set to N/A
Hi there,
Just a short remark: If you wan't to shorten the patch: see #7515.
comment:9 in reply to: ↑ 8 Changed 3 years ago by hivert
Replying to ncohen:
Updated !
It was decided on sage-devel only to put the version and not the date in the message of deprecation aliases. I just uploaded a patch witch does that. Please review. Aside from that
You have a Positive review on trac_7157.patch. You can change the status as soon as you had an eye on my trivial review patch.
Cheers,
Florent
By the way a review on #7515 is welcome ;-)
comment:10 Changed 3 years ago by ncohen
- Status changed from needs_review to positive_review
- Description modified (diff)
comment:11 Changed 3 years ago by mhansen
- Status changed from positive_review to closed
- Reviewers set to Florent Hivert, Mike Hansen
- Resolution set to fixed
- Merged in set to sage-4.3.alpha1
- Authors set to Nathann Cohen
I've added a new trac_7157_review.patch patch with two function calls that were missed.
comment:12 Changed 3 years ago by ncohen
Thank you ! :-)
comment:13 Changed 3 years ago by mvngu
- Summary changed from [with patch, needs review] neighbors_out/in instead of predecessor/successor in DiGraph to neighbors_out/in instead of predecessor/successor in DiGraph
On my side, it passes -testall without any (related) failure.