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:  sage8.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
 Branch set to public/26828_bandwidth_pep8
 Commit set to e0a58e8b3a8f08f0333bc6434f54953f9bd5460b
 Status changed from new to needs_review
comment:2 Changed 3 months ago by
 Commit changed from e0a58e8b3a8f08f0333bc6434f54953f9bd5460b to 1c71b32988e157a64e9f27b3236fcd33174e1ee3
Branch pushed to git repo; I updated commit sha1. New commits:
1c71b32  trac #26828: fix merge conflict with 8.6.beta1

comment:3 Changed 3 months ago by
 Milestone changed from sage8.5 to sage8.6
rebase on 8.6.beta1 and fix a merge conflict.
comment:4 Changed 2 months ago by
 Milestone changed from sage8.6 to sage8.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 sagepending or sagewishlist.
comment:5 Changed 2 months ago by
 Reviewers set to Vincent Klein
comment:6 Changed 2 months ago by
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.
comment:7 Changed 2 months ago by
 Commit changed from 1c71b32988e157a64e9f27b3236fcd33174e1ee3 to e6ba36c362c4892c9b93c3cf9ea836d7b92e02ee
comment:8 Changed 2 months ago by
 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
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
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
 Commit changed from e6ba36c362c4892c9b93c3cf9ea836d7b92e02ee to e6635c4240dd56e2b3d219f1029e8c7d607c819e
Branch pushed to git repo; I updated commit sha1. New commits:
e6635c4  trac #26828: change not i to i == 0

comment:12 Changed 2 months ago by
Thank you for the clarification. I changed to i == 0
to make it clear.
comment:13 Changed 2 months ago by
 Status changed from needs_review to positive_review
Looks good to me.
comment:14 Changed 2 months ago by
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
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
Yes, agreed all of the above :) I'm just making noise ignore me.
comment:17 Changed 2 months ago by
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
 Branch changed from public/26828_bandwidth_pep8 to e6635c4240dd56e2b3d219f1029e8c7d607c819e
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #26828: pep8 cleaning of bandwidth.pyx