Issue #213: Project invalid trajectories inside walkable area #506
Conversation
…correct adjustments
schroedtert
left a comment
schroedtert
left a comment
There was a problem hiding this comment.
This is just the start to get the unit tests running again. Took a brief look at the pre-commit checks, some of them fail due to wrong format of the docstrings, which should be easily fixable. If you need help setting up the pre-commit checks locally let me know. I will have a more in-depth review in the next days.
There was a problem hiding this comment.
Can you move the tests into a preprocessing directory as you did for the implementation, e.g., tests/unit_tests/preprocessing/test_trajectory_projector.py?
| @pytest.fixture | ||
| def setup_trajectory_data(): | ||
| trajectory_data = load_trajectory( | ||
| trajectory_file=pathlib.Path("unit_tests/io/test-data/030_c_56_h0.txt"), |
There was a problem hiding this comment.
The unit tests currently fail, as the path is resolved relative to the working directory. So this may work locally, but fails in the CI pipeline. Better to make the path absolute. This is one way to do this:
| trajectory_file=pathlib.Path("unit_tests/io/test-data/030_c_56_h0.txt"), | |
| trajectory_file=pathlib.Path( pathlib.Path(__file__).parent.parent / "io/test-data/030_c_56_h0.txt"), | |
schroedtert
left a comment
schroedtert
left a comment
There was a problem hiding this comment.
I finally had the time for a more in-depth review, but I skipped the math part, maybe someone else will have a look at that.
There are some general remarks, which we may have to discuss with the others. Personally I would prefer to get the functionality quickly into the main branch and do optimizations then. Otherwise it will take too long to get it included. What do you think.
- You transfer the shapely data structures into float values. Would't it be easier to just keep them and pass them to the functions?
- When dealing with DataFrames, Numpy and Shapely there is usually a way to avoid looping over everything and performing an action on an individual row/data point/geometry. My feeling tells me, that is also possible here, but I would also need a deeper dive into the code test it.
I really like that you already added some unit tests! This gives me a lot of confidence that this works as expected. And as an additional advantage it make optimization and refactoring way easier!
If you don't understand one of the comments or disagree, don't hesitate to contact me (RC or answer to the comment). The comments marked as optional you can resolve yourself if currently too much effort.
There was a problem hiding this comment.
Can you move the test file to a preprocessing/test-data directory?
|
|
||
| The formula for the calculation for the moving points look like: | ||
|
|
||
| x' = (x-a)*((c-b)/(c-a))+b \n |
There was a problem hiding this comment.
For the equations to be displayed correctly, you can take a look[ here:
PedPy/pedpy/methods/speed_calculator.py
Lines 72 to 75 in df39974
In such math contexts you can use LaTeX for displaying the equations.
| back_distance_wall (float): Has to be <0. The distance behind the wall, till | ||
| which the points inside the walls should be corrected. Points, which are | ||
| further inside the walls, are ignored. The parameter is needed for the | ||
| interpolation for the correcting. | ||
| start_distance_wall (float): Has to be >0. The minimum distance, where the points | ||
| should be moved outside the wall | ||
| end_distance_wall (float): Has to be >0 and >= start_distance_wall. Points, which | ||
| lay nearer to the wall than end_distance_wall will be also moved away. A value | ||
| >0 ist needed for the linear interpolation, else the calculations do not work. | ||
| back_distance_obst (float): Has to be <0. Equivalent to backDistance_wall, but the | ||
| value the concerns the obstacles | ||
| start_distance_obst (float): Has to be >0. Equivalent to startDistance_wall, but | ||
| the value concerns the obstacles | ||
| end_distance_obst (float): Has to be >0 and >= start_distance_obst. Equivalent to | ||
| endDistance_wall, but the value concerns the obstacles |
There was a problem hiding this comment.
What are the units of these parameters? Can you add those to the description?
| a is equal to back_distance \n | ||
| b is equal to start_distance \n | ||
| c is equal to end_distance \n | ||
|
|
There was a problem hiding this comment.
This is highly optional!
If you have it already or it is not too difficult to create, a picture/plot showing the projection and the corresponding parameters would be great.
| walkable_area=WalkableArea(walk_area_with_end_distance_all) | ||
| ).index) | ||
|
|
||
| print("Number of points that are going to be corrected: ", |
There was a problem hiding this comment.
Please remove all print statements. If needed, can be turned into a debug log output.
|
|
||
| return x, y | ||
|
|
||
| def point_valid_test( |
There was a problem hiding this comment.
Is this also part of the public interface? If not please add the _ prefix. Also some (may be short) docstring would be great
| back_distance_obst, | ||
| end_distance_obst, | ||
| start_distance_obst, x, y) | ||
| else: #geometry type = wall |
There was a problem hiding this comment.
Please always check here to the desired case and not the negative. This can cause some issues when at some point other types are added. A good way would be:
if (x == type_a):
...
elif (x == type_b):
...
else:
raise InputError("Not supported type...)| if (x == p1x and y == p1y): | ||
| x = x - 0.001 | ||
| if (x == p2x and y == p2y): | ||
| x = x - 0.001 |
There was a problem hiding this comment.
Why do they need to be moved? Would be great if the reason is also stated in the comment.
| f"end_distance_obst has to be >= start_distance_obst") | ||
|
|
||
|
|
||
| def _handle_all_point( |
There was a problem hiding this comment.
Up to you as this is a private interface, but personally I would prefer something like _project_all_points_inside_walkable_area (or shorter)
| return data_trajectories | ||
|
|
||
|
|
||
| def _handle_single_point( |
There was a problem hiding this comment.
Up to you as this is a private interface, but personally I would prefer something like _project_single_inside_walkable_area (or shorter)
2 participants
Added a new preprocessing directory containing the trajectory_projector file.
The function correct_invalid_trajectories identifies every invalid point of a trajectory and moves it outside the geometry.
The direction and the new distance between the point and the wall are computed using trigonometric methods and linear interpolation. Points that lie deeper inside a wall are shifted a shorter distance outward, while points that are already close to the boundary are moved farther.
Closes #213