Opened 6 months ago

Closed 5 months 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) Commit: 3ca6bbeafa63995939bc0cf1d6c16d9c9ae3a7cf
Dependencies: Stopgaps:

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 6 months 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 6 months ago by dcoudert

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

comment:3 Changed 5 months 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 5 months ago by dcoudert

See https://groups.google.com/forum/#!topic/sage-release/ULwr6YtMfnY for a discussion on the build errors with 8.7.beta0.

comment:5 Changed 5 months 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 5 months 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 5 months ago by dcoudert

Perfectly right. It's better like that.

comment:8 Changed 5 months 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 5 months ago by dcoudert

Thanks to both of you for the review !

comment:10 Changed 5 months 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.