Opened 4 years ago

Closed 4 years ago

#26722 closed enhancement (fixed)

py3: work on the sandpile problem

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: Travis Scrimshaw Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 6b63e8d (Commits, GitHub, GitLab) Commit: 6b63e8deb6cf5bde5e76fa09851f974d50533618
Dependencies: Stopgaps:

Status badges

Description (last modified by Travis Scrimshaw)

We change the name of the sink vertices in some sandpile constructions.

Change History (11)

comment:1 Changed 4 years ago by Frédéric Chapoton

Branch: → u/chapoton/26722
Commit: → 8fd6b68531ff343e048a1c96b9183c1a529f26cf

New commits:

​8fd6b68some work on the sandpile problem

comment:2 Changed 4 years ago by Frédéric Chapoton

Milestone: sage-8.5 → sage-8.7

comment:3 Changed 4 years ago by git

Commit: 8fd6b68531ff343e048a1c96b9183c1a529f26cf → ae08212e98267377b1fb0b9d8b3c204e0ec198a8

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

​ae08212Merge branch 'u/chapoton/26722' in 8.7.B1

comment:4 Changed 4 years ago by Frédéric Chapoton

Authors: → Frédéric Chapoton
Branch: u/chapoton/26722 → public/ticket/26722
Commit: ae08212e98267377b1fb0b9d8b3c204e0ec198a8 → 6b63e8deb6cf5bde5e76fa09851f974d50533618
Dependencies: #26630
Status: new → needs_review

Here is a fresh tentative

New commits:

​6b63e8dsome work on sandpiles for python3

comment:5 Changed 4 years ago by Frédéric Chapoton

Cc: Travis Scrimshaw added

green bot, please review

comment:6 Changed 4 years ago by Travis Scrimshaw

Description: modified (diff)
Reviewers: → Travis Scrimshaw
Status: needs_review → positive_review

LGTM.

comment:7 Changed 4 years ago by David Coudert

Also OK on py2 and py3 ;) However, instead of using (-1, -1), one could define a variable sink = (-1, -1) for clarity. I will not fight for it.

I noticed that the sandpile code contains a lot of calls to .vertices() and .edges(), including a .vertices()[0]. So multiple improvements are possible. Also, some random graphs generators could be in digraph_generators.py instead of sandpile.py. Other tickets are needed.

comment:8 in reply to:  7 Changed 4 years ago by Travis Scrimshaw

Replying to dcoudert:

Also OK on py2 and py3 ;) However, instead of using (-1, -1), one could define a variable sink = (-1, -1) for clarity. I will not fight for it.

Followup ticket when the whole module gets a cleaning.

I noticed that the sandpile code contains a lot of calls to .vertices() and .edges(), including a .vertices()[0]. So multiple improvements are possible. Also, some random graphs generators could be in digraph_generators.py instead of sandpile.py. Other tickets are needed.

Yea, it is old code IIRC that has not been really maintained for quite some time.

comment:9 Changed 4 years ago by David Coudert

Let me know if I can help for the next steps. I'm currently blocked in graphs as I'm waiting for the next beta (inclusion of many tickets) and several tickets are waiting for review.

comment:10 Changed 4 years ago by Travis Scrimshaw

I am not able to really contribute code for the near future. The quick-to-medium reviews are all I have time for. :/ Sorry I am not helping more with actual code.

comment:11 Changed 4 years ago by Volker Braun

Branch: public/ticket/26722 → 6b63e8deb6cf5bde5e76fa09851f974d50533618
Resolution: → fixed
Status: positive_review → closed
Note: See TracTickets for help on using tickets.