Opened 4 years ago

Closed 4 years ago

#26834 closed enhancement (fixed)

pep8 cleaning in vertex_separation.pyx

Reported by: David Coudert Owned by:
Priority: major Milestone: sage-8.7
Component: graph theory Keywords:
Cc: Merged in:
Authors: David Coudert Reviewers: Vincent Klein
Report Upstream: N/A Work issues:
Branch: 8e22eb3 (Commits, GitHub, GitLab) Commit: 8e22eb32fe8ee5578866f3b77b0513c82d090614
Dependencies: Stopgaps:

Status badges

Description


Change History (13)

comment:1 Changed 4 years ago by David Coudert

Branch: public/26834_clean_vertex_separation
Commit: fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd
Status: newneeds_review

New commits:

51f7db1trac #26834: pep8 cleaning
fe04f6etrac #26834: fix doctests

comment:2 Changed 4 years ago by vklein

Milestone: sage-8.5sage-8.6
Owner: set to vklein

Review in progress

comment:3 Changed 4 years ago by David Coudert

Thank you. It's a long one, sorry for that.

comment:4 Changed 4 years ago by vklein

Some pep8 remarks

  • 1. Block comment should start with '# ': l275, l283.
  • 2. Blank line contains whitespace:

l431

        [{0, 2, 3, 4}, {0, 1, 2}]
        
    The bags of the path decomposition of a cycle have three vertices each::
  • 3. More than one space before operator:

l466-468

    cdef set seen    = set()  # already treated vertices
    cdef set covered = set()  # vertices in the neighborhood of seen but not in seen
    cdef list bags   = list() # The bags of the path decomposition

pep8 quote :

Avoid ...
...
More than one space around an assignment (or other) operator to align it with 
another.

Yes:

x = 1
y = 2
long_variable = 3

No:

x             = 1
y             = 2
long_variable = 3

l1609 cdef int * prefix = <int *>sig_malloc(n * sizeof(int))

  • 4. At least two spaces before inline comment :

l468 : cdef list bags = list() # The bags of the path decomposition

  • 5. Too many blanklines:

l492, l835, l874, l1013, l1753, l1817, l1846

  • 6. Missing whitspace around operator:

l843 if len(V)==1:

l858 if vsH==-1:

l1611 if prefix==NULL or positions==NULL:

l1660 return (width if width<upper_bound else -1), order

l1774 while i<n:

l1824 if loc_level<=max_prefix_length:

  • 7. lines 851 to 856, replace :
                    vsH,LH = vertex_separation(H, algorithm      = algorithm,
                                               cut_off           = cut_off,
                                               upper_bound       = upper_bound,
                                               verbose           = verbose,
                                               max_prefix_length = max_prefix_length,
                                               max_prefix_number = max_prefix_number)
    

by

                vsH, LH = vertex_separation(H, algorithm=algorithm,
                                            cut_off=cut_off,
                                            upper_bound=upper_bound,
                                            verbose=verbose,
                                            max_prefix_length=max_prefix_length,
                                            max_prefix_number=max_prefix_number)

same thing l1422 to 1427 with

def vertex_separation_BAB(G,
                          cut_off               = None,
                          upper_bound           = None,
                          max_prefix_length     = 20,
                          max_prefix_number     = 10**6,
                          verbose               = False):

l1630 to 1645 with

        width = vertex_separation_BAB_C(H                         = H,
                                        n                         = n,
                                        prefix                    = prefix,
                                        positions                 = positions,
                                        best_seq                  = best_seq,
                                        level                     = 0,
                                        b_prefix                  = bm_pool.rows[3 * n],
                                        b_prefix_and_neighborhood = bm_pool.rows[3 * n + 1],
                                        cut_off                   = cut_off,
                                        upper_bound               = upper_bound,
                                        current_cost              = 0,
                                        bm_pool                   = bm_pool,
                                        prefix_storage            = prefix_storage,
                                        max_prefix_length         = max_prefix_length,
                                        max_prefix_number         = max_prefix_number,
                                        verbose                   = verbose)

l1860 to 1875 with

        cost_i = vertex_separation_BAB_C(H                         = H,
                                         n                         = n,
                                         prefix                    = prefix,
                                         positions                 = positions,
                                         best_seq                  = best_seq,
                                         level                     = loc_level + 1,
                                         b_prefix                  = loc_b_prefix,
                                         b_prefix_and_neighborhood = b_tmp,
                                         cut_off                   = cut_off,
                                         upper_bound               = upper_bound,
                                         current_cost              = delta_i,
                                         bm_pool                   = bm_pool,
                                         prefix_storage            = prefix_storage,
                                         max_prefix_length         = max_prefix_length,
                                         max_prefix_number         = max_prefix_number,
                                         verbose                   = verbose)
  • 8. Don't use spaces around the = sign when used to indicate a keyword argument:

l876 return vertex_separation_exp(G, verbose = verbose)

l879 return vertex_separation_MILP(G, verbosity = (1 if verbose else 0))

l883 max_prefix_number = max_prefix_number)

  • 9. test for membership should be 'x not in E' rather than 'not x in E'

l1239, l1411

comment:5 in reply to:  3 Changed 4 years ago by vklein

Owner: vklein deleted

Replying to dcoudert:

Thank you. It's a long one, sorry for that.

No problem. I don't think that it would be easier to review if splitted into serveral tickets.

comment:6 Changed 4 years ago by vklein

Reviewers: Vincent Klein

comment:7 Changed 4 years ago by git

Commit: fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd8e22eb32fe8ee5578866f3b77b0513c82d090614

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

2e24a77trac #26834: merged with 8.6.rc1
8e22eb3trac #26834: reviewer's comments

comment:8 Changed 4 years ago by David Coudert

I have implemented all your comments. For the long lists of parameters of some methods, I'm not sure it's easier to read this way, but it's not important.

comment:9 in reply to:  8 Changed 4 years ago by vklein

Replying to dcoudert:

For the long lists of parameters of some methods, I'm not sure it's easier to read this way, ...

I totally agree but the thing is, it's highly subjective and therefore different developer will often have different preferred code style. I think Style Guide like pep8 are here to solve this kind of problem.

comment:10 Changed 4 years ago by vklein

Status: needs_reviewpositive_review

Looks good to me.

comment:11 Changed 4 years ago by David Coudert

Thanks.

comment:12 Changed 4 years ago by Erik Bray

Milestone: sage-8.6sage-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:13 Changed 4 years ago by Volker Braun

Branch: public/26834_clean_vertex_separation8e22eb32fe8ee5578866f3b77b0513c82d090614
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.