Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions snippets/graph_neighbourhood.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
""""""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add description in the docstring of what your code is doing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a good name in the start of the code


from itertools import starmap
from operator import le, lt, and_
from pathlib import Path
from os import path
from sys import argv, stdout
from argparse import ArgumentParser

from data import load_graph


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is more space here.

def neighbourhood(coord: tuple[int, int], borders) -> None:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you define what borders should be?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are expecting the function to return None, but the return statement at the end contradicts this

Copy link

@niketagrawal niketagrawal Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

n, s, w, e, nw, ne, sw, se = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(-1, 0),
(1, 0),
(0, -1),
(0, 1),
(-1, -1),
(-1, 1),
(1, -1),
(1, 1),
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), ....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you're totally right and you could actually look at itertools.product in standard Python library to generate those coordinates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was something like

from itertools import product 
coordinates = product((-1, 0, 1), 2)

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)),
Comment on lines +25 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +24 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
Comment on lines +13 to +28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it hard to follow the code without any explanatory comments - but that is (largely?) because I don't program in Python.



if __name__ == "__main__":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider splitting this into smaller functions. At the moment it's not clear to me what this is going to do

parser = ArgumentParser(
description="Determine the list of arcs in the input Graph whose weight is greater or equal to their neighbour coordinates."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

parser.add_argument(
"-i",
"--input",
type=Path,
required=True,
help="Path of the input file containing the list of coordinates to check",
dest="data_filepath",
)
parser.add_argument(
"-o", "--output", type=Path, dest="logfile_output_path", default=None
)

cli_args = parser.parse_args()
adj_matrix = load_graph(cli_args.data_filepath, sparse=False, fillna_with=0)
N = len(adj_matrix)

nn = list()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be put in a callable function as you described it in the pull request description.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use readable variable names.

for u, _ in enumerate(adj_matrix):
Copy link

@niketagrawal niketagrawal Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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))
Comment on lines +52 to +56

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use descriptive variable names?

print(f"Found {len(nn)} edges in the input Graph satisfying the condition.")

if (
cli_args.logfile_output_path is not None
and cli_args.logfile_output_path.exists()
Comment on lines +60 to +61

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

):
logfile = open(cli_args.logfile_output_path, "w")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with statement

else:
logfile = stdout

for edge in nn:
print(f"{edge}", file=logfile)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.