Opened 2 years ago
Closed 2 years 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, GitHub, GitLab) | Commit: | 8e22eb32fe8ee5578866f3b77b0513c82d090614 |
Dependencies: | Stopgaps: |
Description
Change History (13)
comment:1 Changed 2 years ago by
- Branch set to public/26834_clean_vertex_separation
- Commit set to fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- Milestone changed from sage-8.5 to sage-8.6
- Owner changed from (none) to vklein
Review in progress
comment:3 follow-up: ↓ 5 Changed 2 years ago by
Thank you. It's a long one, sorry for that.
comment:4 Changed 2 years ago by
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 years ago by
- 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 years ago by
- Reviewers set to Vincent Klein
comment:7 Changed 2 years ago by
- Commit changed from fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd to 8e22eb32fe8ee5578866f3b77b0513c82d090614
comment:8 follow-up: ↓ 9 Changed 2 years ago by
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 years ago by
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 years ago by
- Status changed from needs_review to positive_review
Looks good to me.
comment:11 Changed 2 years ago by
Thanks.
comment:12 Changed 2 years ago by
- 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 2 years ago by
- Branch changed from public/26834_clean_vertex_separation to 8e22eb32fe8ee5578866f3b77b0513c82d090614
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #26834: pep8 cleaning
trac #26834: fix doctests