Opened 3 months ago

Closed 8 weeks ago

#26828 closed enhancement (fixed)

pep8 cleaning of bandwidth.pyx

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.7
Component: graph theory Keywords:
Cc: tscrim Merged in:
Authors: David Coudert Reviewers: Vincent Klein
Report Upstream: N/A Work issues:
Branch: e6635c4 (Commits) Commit: e6635c4240dd56e2b3d219f1029e8c7d607c819e
Dependencies: Stopgaps:

Description

Clean the file

Change History (18)

comment:1 Changed 3 months ago by dcoudert

  • Branch set to public/26828_bandwidth_pep8
  • Commit set to e0a58e8b3a8f08f0333bc6434f54953f9bd5460b
  • Status changed from new to needs_review

New commits:

e0a58e8trac #26828: pep8 cleaning of bandwidth.pyx

comment:2 Changed 3 months ago by git

  • Commit changed from e0a58e8b3a8f08f0333bc6434f54953f9bd5460b to 1c71b32988e157a64e9f27b3236fcd33174e1ee3

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

1c71b32trac #26828: fix merge conflict with 8.6.beta1

comment:3 Changed 3 months ago by dcoudert

  • Milestone changed from sage-8.5 to sage-8.6

rebase on 8.6.beta1 and fix a merge conflict.

comment:4 Changed 2 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:5 Changed 2 months ago by vklein

  • Reviewers set to Vincent Klein

comment:6 Changed 2 months ago by vklein

Some pep8 remarks

  • 1. Block comment should start with '# ': l104, l112.
  • 2. l246 to 254 change :
    cdef unsigned short ** d                = <unsigned short **> mem.allocarray(n,   sizeof(unsigned short *))
    cdef unsigned short *  distances        = <unsigned short *>  mem.allocarray(n*n, sizeof(unsigned short  ))
    cdef index_t *         current          = <index_t *>         mem.allocarray(n,   sizeof(index_t))
    cdef index_t *         ordering         = <index_t *>         mem.allocarray(n,   sizeof(index_t))
    cdef index_t *         left_to_order    = <index_t *>         mem.allocarray(n,   sizeof(index_t))
    cdef index_t *         index_array_tmp  = <index_t *>         mem.allocarray(n,   sizeof(index_t))
    cdef range_t *         range_arrays     = <range_t *>         mem.allocarray(n*n, sizeof(range_t))
    cdef range_t **        ith_range_array  = <range_t **>        mem.allocarray(n,   sizeof(range_t *))
    cdef range_t *         range_array_tmp  = <range_t *>         mem.allocarray(n,   sizeof(range_t))

into

    cdef unsigned short ** d = <unsigned short **> mem.allocarray(n, sizeof(unsigned short *))
    cdef unsigned short * distances = <unsigned short *> mem.allocarray(n * n, sizeof(unsigned short  ))
    cdef index_t * current = <index_t *> mem.allocarray(n, sizeof(index_t))
    cdef index_t * ordering = <index_t *> mem.allocarray(n, sizeof(index_t))
    cdef index_t * left_to_order = <index_t *> mem.allocarray(n, sizeof(index_t))
    cdef index_t * index_array_tmp = <index_t *> mem.allocarray(n, sizeof(index_t))
    cdef range_t * range_arrays = <range_t *> mem.allocarray(n * n, sizeof(range_t))
    cdef range_t ** ith_range_array = <range_t **> mem.allocarray(n, sizeof(range_t *))
    cdef range_t * range_array_tmp = <range_t *> mem.allocarray(n, sizeof(range_t))
  • 3. l289 to 296 change:
cdef bint bandwidth_C(int n, int k,
                     unsigned short ** d,
                     index_t *         current,         # choice of vertex for the current position
                     index_t *         ordering,        # the actual ordering of vertices
                     index_t *         left_to_order,   # begins with the assigned vertices, ends with the others
                     index_t *         index_array_tmp, # tmp space
                     range_t **        ith_range_array, # array of ranges, for every step of the algorithm
                     range_t *         range_array_tmp):# tmp space

into

cdef bint bandwidth_C(int n, int k,
                      unsigned short ** d,
                      index_t * current,           # choice of vertex for the current position
                      index_t * ordering,          # the actual ordering of vertices
                      index_t * left_to_order,     # begins with the assigned vertices, ends with the others
                      index_t * index_array_tmp,   # tmp space
                      range_t ** ith_range_array,  # array of ranges, for every step of the algorithm
                      range_t * range_array_tmp):  # tmp space
  • 4. At least two spaces before inline comment :

l299 cdef int pi # the position l300 cdef int vi # the vertex

  • 5. Missing whitespace around operator:

l354 and l355 ith_range_array[i+1]

Python question

-            if i == 0:
+            if not i:

What is the reason for using not i rather than i == 0 ?

not i mean i == 0 or i is None is it ok to do the same thing in both case ?
For what i see i is None should never happen but if it do i don't think it should pass silently.

Last edited 2 months ago by vklein (previous) (diff)

comment:7 Changed 2 months ago by git

  • Commit changed from 1c71b32988e157a64e9f27b3236fcd33174e1ee3 to e6ba36c362c4892c9b93c3cf9ea836d7b92e02ee

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

6fdd508trac #26828: Merged with 8.6
e6ba36ctrac #26828: reviewer's comments

comment:8 Changed 2 months ago by dcoudert

  • Cc tscrim added

Thank you for your help in reviewing this ticket.

I'm ccing Travis as he may answer the not i instead of i == 0 question.

comment:9 Changed 2 months ago by jdemeyer

Here we have cdef int i so it does not matter at all whether you write i == 0 or not i. It's just a matter of style.

The advantage of i == 0 is that it makes it clear that you're really testing for 0.

comment:10 Changed 2 months ago by vklein

I am ok with ​e6ba36c. Concerning the not i vs i == 0 it's up to you dcoudert you can set the ticket to positive review if you want to keep not i.

comment:11 Changed 2 months ago by git

  • Commit changed from e6ba36c362c4892c9b93c3cf9ea836d7b92e02ee to e6635c4240dd56e2b3d219f1029e8c7d607c819e

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

e6635c4trac #26828: change not i to i == 0

comment:12 Changed 2 months ago by dcoudert

Thank you for the clarification. I changed to i == 0 to make it clear.

comment:13 Changed 2 months ago by vklein

  • Status changed from needs_review to positive_review

Looks good to me.

comment:14 Changed 2 months ago by embray

I'm not wild about the change

-    cdef unsigned short ** d                = <unsigned short **> mem.allocarray(n,   sizeof(unsigned short *))
-    cdef unsigned short *  distances        = <unsigned short *>  mem.allocarray(n*n, sizeof(unsigned short  ))
-    cdef index_t *         current          = <index_t *>         mem.allocarray(n,   sizeof(index_t))
-    cdef index_t *         ordering         = <index_t *>         mem.allocarray(n,   sizeof(index_t))
-    cdef index_t *         left_to_order    = <index_t *>         mem.allocarray(n,   sizeof(index_t))
-    cdef index_t *         index_array_tmp  = <index_t *>         mem.allocarray(n,   sizeof(index_t))
-    cdef range_t *         range_arrays     = <range_t *>         mem.allocarray(n*n, sizeof(range_t))
-    cdef range_t **        ith_range_array  = <range_t **>        mem.allocarray(n,   sizeof(range_t *))
-    cdef range_t *         range_array_tmp  = <range_t *>         mem.allocarray(n,   sizeof(range_t))
-
-    cdef int i,j,kk
+    cdef unsigned short ** d = <unsigned short **> mem.allocarray(n, sizeof(unsigned short *))
+    cdef unsigned short * distances = <unsigned short *> mem.allocarray(n*n, sizeof(unsigned short))
+    cdef index_t * current = <index_t *> mem.allocarray(n, sizeof(index_t))
+    cdef index_t * ordering = <index_t *> mem.allocarray(n, sizeof(index_t))
+    cdef index_t * left_to_order = <index_t *> mem.allocarray(n, sizeof(index_t))
+    cdef index_t * index_array_tmp  = <index_t *> mem.allocarray(n, sizeof(index_t))
+    cdef range_t * range_arrays = <range_t *> mem.allocarray(n*n, sizeof(range_t))
+    cdef range_t ** ith_range_array  = <range_t **> mem.allocarray(n, sizeof(range_t *))
+    cdef range_t * range_array_tmp  = <range_t *> mem.allocarray(n, sizeof(range_t))

As much as I like to stick to pep8, and have sometimes made changes like this myself, between the two I genuinely find the original formatting more readable, in this particular case. Nevertheless, it's not that important either way.

comment:15 Changed 2 months ago by vklein

I agree with you for the readability.
But i also assume that readability is something highly subjective.

Either way the meaning of my pep8 remarks were "If the goal of the ticket is to be as close as possible to pep8 you should do this" rather than "This is a problem blocking the ticket".

comment:16 Changed 2 months ago by embray

Yes, agreed all of the above :) I'm just making noise ignore me.

comment:17 Changed 2 months ago by dcoudert

I also find the first presentation easier to read, but I will not fight for it. At least we have globally improved the presentation of this file.

comment:18 Changed 8 weeks ago by vbraun

  • Branch changed from public/26828_bandwidth_pep8 to e6635c4240dd56e2b3d219f1029e8c7d607c819e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.