Opened 3 years ago

Closed 3 years ago

#26829 closed enhancement (fixed)

improve cutwidth.pyx

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.7
Component: graph theory Keywords:
Cc: Merged in:
Authors: David Coudert Reviewers: Kevin Dilks, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 3ca6bbe (Commits, GitHub, GitLab) Commit: 3ca6bbeafa63995939bc0cf1d6c16d9c9ae3a7cf
Dependencies: Stopgaps:

Status badges

Description (last modified by dcoudert)

In this ticket, we:

  • avoid recursive calls in method cutwidth
  • avoid using .vertices() in all methods
  • do some pep8 cleaning

Change History (10)

comment:1 Changed 3 years ago by dcoudert

  • Branch set to public/26829_cutwidth
  • Commit set to b7abfc802af4077f39b242414966de1b3ffd7fc3

New commits:

303b896avoid recursive calls in cutwidth
6404223avoid using .vertices() in cutwidth.pyx
d57b5f4pep8 cleaning in method width_of_cut_decomposition
f5a479fpep8 in method cutwidth
656f8f3pep8 in cutwidth_dyn
f59a401pep8 in exists
b7abfc8pep8 in cutwidth_MILP

comment:2 Changed 3 years ago by dcoudert

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

comment:3 Changed 3 years ago by kdilks

Everything tentatively looks good to me. Not sure why most recent patchbots are having build errors (builds and tests fine of this machine), will run patchbot on a different machine soon to double check.

comment:4 Changed 3 years ago by dcoudert

See!topic/sage-release/ULwr6YtMfnY for a discussion on the build errors with 8.7.beta0.

comment:5 Changed 3 years ago by tscrim

Do we need this to explicitly be a list:

-    V = G.vertices()
+    V = list(G)
     # All vertices at different positions
     for v in V:

In particular, could we just do

-    V = G.vertices()
     # All vertices at different positions
-    for v in V:
+    for v in G:

comment:6 Changed 3 years ago by git

  • Commit changed from b7abfc802af4077f39b242414966de1b3ffd7fc3 to 3ca6bbeafa63995939bc0cf1d6c16d9c9ae3a7cf

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

4d18612trac #26829: Merged with 8.7.beta0
3ca6bbetrac #26829: avoid a creation of list of vertices

comment:7 Changed 3 years ago by dcoudert

Perfectly right. It's better like that.

comment:8 Changed 3 years ago by tscrim

  • Milestone changed from sage-8.5 to sage-8.7
  • Reviewers set to Kevin Dilks, Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:9 Changed 3 years ago by dcoudert

Thanks to both of you for the review !

comment:10 Changed 3 years ago by vbraun

  • Branch changed from public/26829_cutwidth to 3ca6bbeafa63995939bc0cf1d6c16d9c9ae3a7cf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.