Conversation
| @@ -0,0 +1,68 @@ | |||
| """""" | |||
There was a problem hiding this comment.
Need a good name in the start of the code
| @@ -0,0 +1,68 @@ | |||
| """""" | |||
There was a problem hiding this comment.
Please add description in the docstring of what your code is doing.
| lambda l: and_(*starmap(le, zip((0, 0), l))) | ||
| and and_(*starmap(lt, zip(l, borders))), | ||
| map(lambda n: tuple(map(sum, zip(coord, n))), (n, s, w, e, nw, ne, sw, se)), |
There was a problem hiding this comment.
Lambda functions can be difficult to understand for a reader. I would prefer it if you created small functions whose names describe what the function is doing.
|
|
||
| from data import load_graph | ||
|
|
||
|
|
| def neighbourhood(coord: tuple[int, int], borders) -> None: | ||
| n, s, w, e, nw, ne, sw, se = [ | ||
| (-1, 0), | ||
| (1, 0), | ||
| (0, -1), | ||
| (0, 1), | ||
| (-1, -1), | ||
| (-1, 1), | ||
| (1, -1), | ||
| (1, 1), | ||
| ] | ||
| return filter( | ||
| lambda l: and_(*starmap(le, zip((0, 0), l))) | ||
| and and_(*starmap(lt, zip(l, borders))), | ||
| map(lambda n: tuple(map(sum, zip(coord, n))), (n, s, w, e, nw, ne, sw, se)), | ||
| ) |
There was a problem hiding this comment.
I find it hard to follow the code without any explanatory comments - but that is (largely?) because I don't program in Python.
| return filter( | ||
| lambda l: and_(*starmap(le, zip((0, 0), l))) | ||
| and and_(*starmap(lt, zip(l, borders))), | ||
| map(lambda n: tuple(map(sum, zip(coord, n))), (n, s, w, e, nw, ne, sw, se)), |
There was a problem hiding this comment.
I would find this clearer if you wrapped this filter call in its own function whose name describes what it is doing. Then return the result of that function call.
| from data import load_graph | ||
|
|
||
|
|
||
| def neighbourhood(coord: tuple[int, int], borders) -> None: |
There was a problem hiding this comment.
Please could you define what borders should be?
| from data import load_graph | ||
|
|
||
|
|
||
| def neighbourhood(coord: tuple[int, int], borders) -> None: |
There was a problem hiding this comment.
Looks like you are expecting the function to return None, but the return statement at the end contradicts this
| (-1, 1), | ||
| (1, -1), | ||
| (1, 1), | ||
| ] |
There was a problem hiding this comment.
It's better to define a "list" to show the numbers or we can use an algorithm in another def to create these numbers like (-1,0), (1,0), ....
There was a problem hiding this comment.
Well, you're totally right and you could actually look at itertools.product in standard Python library to generate those coordinates.
There was a problem hiding this comment.
What I meant was something like
from itertools import product
coordinates = product((-1, 0, 1), 2)
| if __name__ == "__main__": | ||
| parser = ArgumentParser( | ||
| description="Determine the list of arcs in the input Graph whose weight is greater or equal to their neighbour coordinates." | ||
| ) |
There was a problem hiding this comment.
I prefer to define a variable for "Determine the list of arcs in the input Graph whose weight is greater or equal to their neighbour coordinates" out of the if condition.
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
I would consider splitting this into smaller functions. At the moment it's not clear to me what this is going to do
steven-wooding
left a comment
steven-wooding
left a comment
There was a problem hiding this comment.
It's hard to review Python code when you don't program in Python.
|
|
||
| if __name__ == "__main__": | ||
| parser = ArgumentParser( | ||
| description="Determine the list of arcs in the input Graph whose weight is greater or equal to their neighbour coordinates." |
There was a problem hiding this comment.
Could you clarify whether an 'arc' is another term for 'edge' or has some special technical meaning - ie is it a special type of 'edge'.
| logfile = stdout | ||
|
|
||
| for edge in nn: | ||
| print(f"{edge}", file=logfile) |
There was a problem hiding this comment.
Is it possible to pass a file path to 'logfile' or is only a filename acceptable - it might be worth renaming as logfilepath if a file path is acceptable.
| for u, _ in enumerate(adj_matrix): | ||
| for v, w in enumerate(adj_matrix[u]): | ||
| for (i, j) in neighbourhood((u, v), borders=(N, N)): | ||
| if w >= adj_matrix[i][j]: | ||
| nn.append((u, v)) |
There was a problem hiding this comment.
Could you use descriptive variable names?
| cli_args.logfile_output_path is not None | ||
| and cli_args.logfile_output_path.exists() |
There was a problem hiding this comment.
I would consider wrapping this into a small function that validates whether a valid file path has been provided. Then you could use the same function to validate the input file path too.
| cli_args.logfile_output_path is not None | ||
| and cli_args.logfile_output_path.exists() | ||
| ): | ||
| logfile = open(cli_args.logfile_output_path, "w") |
There was a problem hiding this comment.
Consider:
- whether you want to 'append' instead of 'write'
- you don't close this file. Consider whether you want to refactor so that the write statement is called within a
withstatement
| N = len(adj_matrix) | ||
|
|
||
| nn = list() | ||
| for u, _ in enumerate(adj_matrix): |
There was a problem hiding this comment.
Please consider encapsulating the logic in the for loop in a function with a relevant name. This will help improve the readability of the code.
|
|
||
|
|
||
| def neighbourhood(coord: tuple[int, int], borders) -> None: | ||
| n, s, w, e, nw, ne, sw, se = [ |
There was a problem hiding this comment.
Please rename the variables to give them a more meaningful context. For e.g., 'n' could be renamed to north, nw could be renamed to northwest assuming that these variables represent the directions.
| from data import load_graph | ||
|
|
||
|
|
||
| def neighbourhood(coord: tuple[int, int], borders) -> None: |
There was a problem hiding this comment.
Including a docstring for the function renders readability to the code. You can consider using the Numpy-doc standard for writing a docstring. You can reuse the function description in the PR to put in the docstring.
Good to see that the type is annotated for the first function argument; you could do the same for the second argument 'borders' as well.
| adj_matrix = load_graph(cli_args.data_filepath, sparse=False, fillna_with=0) | ||
| N = len(adj_matrix) | ||
|
|
||
| nn = list() |
There was a problem hiding this comment.
This should be put in a callable function as you described it in the pull request description.
| adj_matrix = load_graph(cli_args.data_filepath, sparse=False, fillna_with=0) | ||
| N = len(adj_matrix) | ||
|
|
||
| nn = list() |
There was a problem hiding this comment.
Try to use readable variable names.
8 participants
This PR will add to the repo a Python module to determine the list of arcs in a graph whose weight is greater or equal to their corresponding neighbour coordinates.
The module includes a
neighbourhoodfunction that determines the neighbourhood of each node which is independent, and importable from other third-party code.The module also contains a main section to run and test the program that is working with some input file representing the connections between nodes in a graph.
Note:
I'd like to receive comments and suggestions about the organisation of the code, and whether you think my code could be improved in any way.
This is my first contribution to the project, and I am not very experienced in Python.
I've read guidelines on how to contribute to the repo, but I'd love to receive any comments you might have.