Opened 11 months ago

Closed 11 months ago

#26621 closed enhancement (fixed)

clean bliss.pyx

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.5
Component: graph theory Keywords: py3, graph
Cc: tscrim, dimpase Merged in:
Authors: David Coudert Reviewers: Travis Scrimshaw, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 92c3509 (Commits) Commit: 92c3509f81a65bd705ece438714215c845552b62
Dependencies: Stopgaps:

Description

PEP8 and avoid using .edges() and .vertices()

Change History (10)

comment:1 Changed 11 months ago by dcoudert

  • Branch set to public/26621_bliss
  • Cc tscrim dima added
  • Commit set to 9e0691ee5c7897f8d4ceca97e23c6bb106ed9992
  • Status changed from new to needs_review

@dima: could you check that variable logLnr is always assigned a value in methods bliss_graph_from_labelled_edges and bliss_digraph_from_labelled_edges. I seems that it might be used uninitialized...

New commits:

​9e0691etrac #26621: clean bliss.pyx

comment:2 Changed 11 months ago by tscrim

  • Cc dimpase added; dima removed

Dima, see comment:1.

David, Micro-optimization, but since you know cdef list done = [False] * n will be a length n array of bint's, you are better to directly allocate such an array (Cython generates better C code and you do not have the overhead of list).

comment:3 Changed 11 months ago by dcoudert

  • Status changed from needs_review to needs_work

I will do that. I also did a mistake in initializations. I will fix that.

comment:4 Changed 11 months ago by git

  • Commit changed from 9e0691ee5c7897f8d4ceca97e23c6bb106ed9992 to 1a59d4f412a3a0c2919a4c6f0846bcafb10c679a

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

​1a59d4ftrac #26621: improvements

comment:5 Changed 11 months ago by dcoudert

  • Status changed from needs_work to needs_review

Ready for review.

comment:6 Changed 11 months ago by dimpase

It seems to be a good idea to declare logLnr as 

cdef int logLnr = 0

(in two places in src/sage/graphs/bliss.pyx)

comment:7 Changed 11 months ago by git

  • Commit changed from 1a59d4f412a3a0c2919a4c6f0846bcafb10c679a to 92c3509f81a65bd705ece438714215c845552b62

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

​92c3509trac #26621: initialize logLnr to 0

comment:8 Changed 11 months ago by dcoudert

It's certainly better like that. Thanks.

comment:9 Changed 11 months ago by tscrim

  • Reviewers set to Travis Scrimshaw, Dima Pasechnik
  • Status changed from needs_review to positive_review

Thank you Dima. Everything else LGTM.

comment:10 Changed 11 months ago by vbraun

  • Branch changed from public/26621_bliss to 92c3509f81a65bd705ece438714215c845552b62
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.