Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#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 Merged in: sage-4.3.alpha1
Authors: Nathann Cohen Reviewers: Florent Hivert, Mike Hansen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ncohen)

This patch replaces the names successors/iterators by neighbors_in and neighbors_out.

THIS PATCH DEPENDS ON #7515 !!!!

Attachments (2)

trac_7157.patch (10.3 KB) - added by ncohen 9 years ago.
trac_7157_review.patch (2.6 KB) - added by mhansen 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by ncohen

  • Status changed from new to needs_review

On my side, it passes -testall without any (related) failure.

comment:2 Changed 9 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: Changed 9 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 9 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 9 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 9 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:7 Changed 9 years ago by ncohen

Thank you very much !!! :-)

comment:8 follow-up: Changed 9 years ago by ncohen

  • Description modified (diff)

Updated !

Changed 9 years ago by ncohen

comment:9 in reply to: ↑ 8 Changed 9 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 9 years ago by ncohen

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

Changed 9 years ago by mhansen

comment:11 Changed 9 years ago by mhansen

  • Authors set to Nathann Cohen
  • Merged in set to sage-4.3.alpha1
  • Resolution set to fixed
  • Reviewers set to Florent Hivert, Mike Hansen
  • Status changed from positive_review to closed

I've added a new trac_7157_review.patch patch with two function calls that were missed.

comment:12 Changed 9 years ago by ncohen

Thank you ! :-)

comment:13 Changed 9 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
Note: See TracTickets for help on using tickets.