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: |
Description
Change History (13)
comment:1 Changed 4 years ago by
Branch: | → public/26834_clean_vertex_separation |
---|---|
Commit: | → fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd |
Status: | new → needs_review |
comment:2 Changed 4 years ago by
Milestone: | sage-8.5 → sage-8.6 |
---|---|
Owner: | set to vklein |
Review in progress
comment:4 Changed 4 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 Changed 4 years ago by
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
Reviewers: | → Vincent Klein |
---|
comment:7 Changed 4 years ago by
Commit: | fe04f6ee093dd548fedb1f3aaedad88b39bb6ccd → 8e22eb32fe8ee5578866f3b77b0513c82d090614 |
---|
comment:8 follow-up: 9 Changed 4 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 Changed 4 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:12 Changed 4 years ago by
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
Branch: | public/26834_clean_vertex_separation → 8e22eb32fe8ee5578866f3b77b0513c82d090614 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
trac #26834: pep8 cleaning
trac #26834: fix doctests