Peer review

[jakubgrad]

Hi! I downloaded your repository at 3:30 pm 16.04.24 I read through it and run your program. Here's what I have to say:

Documentation

• Inside Testausdokumentti I recommend you include a link to an online test coverage report, so that's it's possible to see which parts of the code were tested. To do this, you can create you own Github page from a repository. E.g., I keep my newest coverage report inside coverage/aineo/week4 and because it's a github page, it's also accessible here.
  
• I think you forgot to mention in the README.md that running poetry shell is necessary.
• minor mistake, but in README.md it should be poetry run python src/main.py and not poetry run python src\main.py
• I wasn't sure what a "scenario" is. Is it just chosen start and end coordinates? In general, I think it would be nice to include more explanations in the README.md, although to some people a scenario is pretty self-explanatory

Program ergonomy

• I would also really recommend using the Invoke library for things like starting the program, testing your code, and creating coverage reports. It's really easy to use create your own custom commands and avoid typing long lines of arguments in the command line, and there's a convenient instructions for it in the software development course here. The end result looks of the config file looks like this, and it also allows me to view the coverage report online and see the code with highlighted tested and untested parts with a simple command poetry run invoke coverage-report:
  It's also nice to have default option, so that e.g. you can press enter when it asks Kirjoita kartan nimi, jolla haluat testata polunetsintä algoritmeja: and it would just choose the first map

Refactoring main.py

• The main.py file seems to be longer than necessary. I would recommend creating a services directory and putting all the functionality related to actually running the algorithms in a file algorithm_service, and expanding console_io.py to handle more logic related to user input. This is because main.py as of now has things that seem unrelated to each other, like:

  
  def list_map_names     # run just once when using command line
  def  display_formed_img # right next to it, takes care of showing the final result of running the algorithm
  ...
  def  check_commandline_argument_and_get_map_to_test # used to prompt user for the name of the map
  ...
  def main(io, cl_args) # which is different to __main__ but actually takes care of starting the command line interaction

This makes it harder to understand the code because UI and backend logic are written in a single pile of code.
I don't really know the best way to refactor the code. I think the easiest option would be to move the `main(io, cl_args)` function into the ui directory together with logic handling the user interaction. Logic related to algorithms could be moved to `algorithm_service` file, and `main.py` could be left with with just a few imports and injection dependencies fo calling main(io, cl_args) inside ui directory.<br/>

# Refactoring assets_io.py
I think `assets_io` could also use a bit of refactoring. I was surprised to find there e.g. the function `get_path_between_two_nodes`.  It sounds like the kind of function that's not directly related to input/output though I also don't understand the code very well and can easily be wrong in this case. After reading algorithm implementations inside `algorithms` directory and coming back here I'm even less sure what it does. Functions like `read_map` and `read_scenario` fit perfectly well here, but `get_path_between_two_nodes` seems to be doing an important part of the algorithms:

    while path_node != start:
        x1, y1 = path_node
        _, path_node = visited[path_node]
        x2, y2 = path_node
        # Solmut samalla rivillä
        if x1 == x2:
            for i in range(abs(y1-y2)):
                path.add((x1,min(y1,y2)+i))
        # Solmut samassa sarakkeessa

This is something that I would definitely expect to see inside the JPS file and not in assets_io.py. I think it's a very neat code, and as a person who's also comparing JPS and Dijkstra for the course I'm impressed with how simple the handling of path creation is here.  It's very minimalistic and seems to work perfectly well in simulations, but it's just the kind of the code that I would expect to see elsewhere. However, I have only seen `get_path_between_two_nodes` referenced inside the assets_io.py inside  `draw_path_onto_map`, so my guess is that it's getting the path between two nodes is only necessary if you want to print the result to the user, but not if you want to get best performance. But this should perhaps be clearer to the code reader.<br/>
Also, I don't see any treatment for the A_star algorithm here. I mean, the [`get_path_between_two_nodes`](https://github.com/Wincewind/tiralabra/blob/main/src/assets_io.py#L86-L122) handles the case that `if algorithm != "jps"` and an `else`, but there is no handling for A_star. I'm sure it is somewhere since I tested the programme and it works, but I just can't find it inside the function that supposedly would have it. <br/><br/>

# Naming conventions
Looking at the implementation of algorithms I gotta say they make a lot of sense to me. A_star is very consise and JPS looks very intuitive to a person who's read the JPS paper, at least on the high level. I'm not sure if below is even a good suggestion to make here since perhaps the code is small and understandable enough, but I'm usually a proponent of creating very specific use functions and giving very descriptive variable names. In case of `jps.py`:<br/>

for d, _ in graph.nodes[current_node].items(): jump_point = _jump(current_node, d, goal, graph) if jump_point[0] is not None: successors.append(jump_point)

Which is [here](https://github.com/Wincewind/tiralabra/blob/main/src/algorithms/jps.py#L38-L41), inside `_identify_successors` I have kind of guessed that d means distance, and so I suppose you are extracting the information of in which directions from the current node can we find neighbours. Then, you create a jump point in that direction, and if the jump point[0] is not none, you assume that there is something interesting to explore there and you add it to sucessors. To me, an easier way to understand this piece would be if `d` was renamed to `distance` and graph.nodes[current_node].items() would have a nicer name. Perhaps a node could have it's own list called neighbours? Then graph.nodes[current_node].neighbours maybe would be more descriptive. Similarly, `if jump_point[0] is not None:` is very, very implementation dependent and a slight refactoring of the jump_point would uppend it. <br/>
[Similarly](https://github.com/Wincewind/tiralabra/blob/main/src/algorithms/jps.py#L80-L81):

if direction % 2 != 0:
    directions = SURROUNDING_DIRECTIONS[direction]

After reading how directions are numbered it's pretty obvious that here you are checking whether the scan proceeds horizontally or diagonally, but even making a function `is_diagonal(self, direction)` which does the same mathematical operation and expands the code perhaps four fold for the same operation would nevertheless help to understand the code at this point. 
<br/>

# Nice to haves
- It would be nice to see the app with a graphical interface

# Positive feedback
I'm impressed! You managed to implement three algorithms and a common visualization for each, as well as conversion from image into a 2D grid, which I gave up on the very start of the project. The CLI doesn't seem to have any bugs and the algorithms seem to run very fast. The visualization, especially for maze, is pretty intuitive and the purpose of the program is clear from the start. <br/>
The code for A_star and JPS is pretty amazing. It's very neat that you include references to the JPS paper inside the docstrings, since it's easy to verify that the algorithm you've implemented really is JPS.
- It's interesting that the algorithms performed better or worse relative to each other depending on the maps and scenarios that I run.
- It's nice that there is both unittesting and integration testing mentioned in testausdokumentti
- I think the final version of the repository would be a very functional tool for comparing the three algorithms to each other, especially since your code easily handles huge `.map` files already.

[Wincewind]

Thanks for the extensive feedback! Here are some comments on the points raised:

Documentation

• I actually have the html test coverage in CodeCov, which the badge in readme links to, but for clarity I made sure to add it to the Testausdokumentti as well!
• Running poetry shell isn't actually necessary if you start the command with "poetry run" in which case the installed venv is used. I had this included earlier in the readme, but for some reason on Windows when you have the venv active, the cmd prompt won't let you scroll to previously executed commands and they had to be retyped. Thus I tried to encourage to rather start the commands with "poetry run".
• Thanks for catching the slashes! Once again a stupid error from developing with Windows..

Program ergonomy

• I was considering adding the Invoke lib earlier in the project, but decided against it since I don't feel it would add much. Yes it could make the commands for testing/coverage more streamlined, but I already have automated testing and coverage creation done by GH actions so I feel I have this covered. With the main program, my aim is to give the user ability to customise the simulation as freely as possible and I think restricting options with pre-made invokes would be counter-productive.

Refactoring

• Good insights and I think you're correct with moving stuff to separate services modules. Assets_io is basically the service for handing the assets, but it has become so bloated that a separate visualisation_service would be appreciated.
• I apologise about the confusion with the get_path_between_two_nodes naming, I wasn't really considering the context when trying to name it.. The algorithms gather the found path into the visited dict so that at the end of an algorithm's pathfinding, the coords of the goal node return the distance to that node and the node which we've arrived to the goal node to, from the visited dict. We use this dict to gather the pixel coordinates of the found path so it can be drawn to the png of the map. In Dijkstra and A*, since the neighbouring node always follows/precedes the next one in the path, the path is easy to form:

  https://github.com/Wincewind/tiralabra/blob/774cc2a98756e85f0eafd045cd17e53ad1d29c82/src/assets_io.py#L102-L104 but with JPS, since the previous node in the path is actually a jump point, there can be several coordinates missing between two jump points if we want to draw a solid path. I've abstracted the use from "get_path_between_goal_and_start", since I'll be able to use it now to also form a path that isn't necessarily complete.

Thanks once again for all the pointers and if you're interested, I managed to add a gif generation option to the program slightly after the version downloaded for review :)!

[jakubgrad]

Huh, the CodeCov tool looks pretty impressive! I haven't seen it nor Github badges before.
Regarding poetry shell I'm not sure anymore if poetry run runs in virtual environment by default on the university virtual machine, but it's nice that now it's in the README anyway.
I suppose Invoke isn't useful after all, since as you mentioned you use Github actions and actually passing command to a program through Invoke can turn out to be pretty cumbersome
The gif generation that you implemented looks really cool! I think it's worth mentioning in the README that gifs are possible to see only inside the output directory, or at I don't think I saw any animation happening while running the program.
Good job!