Opened 4 months ago

Closed 8 weeks ago

#26834 closed enhancement (fixed)

pep8 cleaning in vertex_separation.pyx

Reported by: dcoudert 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) Commit: 8e22eb32fe8ee5578866f3b77b0513c82d090614
Dependencies: Stopgaps:

Description


Change History (13)

comment:1 Changed 4 months ago by dcoudert

  • Branch set to public/26834_clean_vertex_separation
  • Commit set to fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 2 months ago by vklein

  • Milestone changed from sage-8.5 to sage-8.6
  • Owner changed from (none) to vklein

Review in progress

comment:3 follow-up: Changed 2 months ago by dcoudert

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

comment:4 Changed 2 months 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 2 months ago by vklein

  • Owner changed from vklein to (none)

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 2 months ago by vklein

  • Reviewers set to Vincent Klein

comment:7 Changed 2 months ago by git

  • Commit changed from fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd to 8e22eb32fe8ee5578866f3b77b0513c82d090614

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 follow-up: Changed 2 months ago by dcoudert

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 2 months 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 2 months ago by vklein

  • Status changed from needs_review to positive_review

Looks good to me.

comment:11 Changed 2 months ago by dcoudert

Thanks.

comment:12 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:13 Changed 8 weeks ago by vbraun

  • Branch changed from public/26834_clean_vertex_separation to 8e22eb32fe8ee5578866f3b77b0513c82d090614
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.