Opened 4 years ago

Closed 4 years ago

# pep8 cleaning in vertex_separation.pyx

Reported by: Owned by: David Coudert major sage-8.7 graph theory David Coudert Vincent Klein N/A 8e22eb3 8e22eb32fe8ee5578866f3b77b0513c82d090614

### comment:1 Changed 4 years ago by David Coudert

Branch: → public/26834_clean_vertex_separation → fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd new → needs_review

New commits:

 ​51f7db1 `trac #26834: pep8 cleaning` ​fe04f6e `trac #26834: fix doctests`

### comment:2 Changed 4 years ago by vklein

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

Review in progress

### comment:3 follow-up:  5 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

• 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

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

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

 ​2e24a77 `trac #26834: merged with 8.6.rc1` ​8e22eb3 `trac #26834: reviewer's comments`

### comment:8 follow-up:  9 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

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_review → positive_review

Looks good to me.

Thanks.

### comment:12 Changed 4 years ago by Erik Bray

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

Branch: public/26834_clean_vertex_separation → 8e22eb32fe8ee5578866f3b77b0513c82d090614 → fixed positive_review → closed
Note: See TracTickets for help on using tickets.