-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Vadere trajectory loader #382
base: main
Are you sure you want to change the base?
Add Vadere trajectory loader #382
Conversation
…ce file (created with Vadere 3.0, d79a4bfd)
Removed commented code
* Add scenario and trajectory file * Update user_guide.ipynb
This allows us to explicitly handle floating point errors that occur when transforming walkable areas and shapely polygons
Resolve merge conflicts
Thanks for your contribution, we are really looking forward to support Vadere trajectory files! If you have any question or need some assistance, don't hesitate to ask. Especially when our contributing workflow is not clear (you are the first external contributor). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, I just have some points which I want to clarify, before merging this. If you could reply to these remarks, that would be great.
On a different note, you are a bit unfortunate, as we just changed our linting and formatting to ruff. Running the checks locally, yields around 40 places where (mostly minor) remarks are given. If you need any assistance in getting your state to the current main or dealing with the linter warnings just let me know. As I am not that much into the Vadere trajectory format, I'm trusting you, that you kept all the edge cases in mind and handled. And it's great that you added test cases for your changes!
Thanks again for contributing and this great PR. I am hopeful, that we will be able to merge this soon!
def load_trajectory_from_vadere( | ||
*, | ||
trajectory_file: pathlib.Path, | ||
frame_rate: float = 24.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS this the default frame_rate
in Vadere?
This function reads a traj file containing trajectory data from Vadere simulations and | ||
converts it into a :class:`~trajectory_data.TrajectoryData` object which can be used for | ||
further analysis and processing in the *PedPy* framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR you mention that only Vadere version 3.0 traj files are supported. This should also be mentioned in the documentation. Would it be possible to enforce this? Or what happens with any traj files before 3.0?
_log.warning( | ||
f"Trajectory of pedestrian {str(ped_id)} is too short in time " | ||
f"to be captured by the chosen frame rate of {str(frame_rate)}. " | ||
f"Therefore, this trajectory will be ignored." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand this correctly, when a trajectory is too short to convert from the event-based data to a frame-rate based data, it will be ignored? Is this the intended behavior?
For me, a single invalid trajectory would render the whole file invalid and the user needs to update their trajectory file. This is the current philosophy of PedPy, if during the reading of the data anything is incorrect, an exception is raised and the user has to correct the data. This way, we make sure, that PedPy does not try to be smarter than the user to avoid any unexpected results when such warning messages get ignored. Or would this workflow not work with Vadere files?
I'm open to the warnings, but wanted to express my concerns
def load_walkable_area_from_vadere_scenario( | ||
vadere_scenario_file: pathlib.Path, | ||
margin: float = 0, | ||
decimals: int = 6, | ||
) -> WalkableArea: | ||
"""Loads the walkable area from the Vadere scenario file as :class:`~geometry.WalkableArea`. | ||
|
||
.. note:: | ||
Obstacles in the scenario files are not allowed to overlap with other obstacles or the | ||
bounding box. Merge overlapping obstacles in Vadere before loading the scenario into PedPy. | ||
|
||
Args: | ||
vadere_scenario_file: Vadere scenario file (json format) | ||
margin: Increases the walkable area by the value of margin to avoid that the topography | ||
bound touches obstacles because shapely Polygons used in PedPy do not allow this. | ||
By default (margin = .0), the bound of the walkable area in PedPy coincides with the | ||
inner bound of the bounding box (obstacle) in Vadere. PedPy cannot process the case | ||
where obstacles touch the bounding box defined in Vadere. To avoid errors, either | ||
increase the value of margin (e.g. to 1e-3) or make sure that the obstacles in | ||
Vadere do not touch the bounding box. | ||
decimals: Integer defining the decimals of the coordinates of the walkable area | ||
|
||
Returns: | ||
WalkableArea: :class:`~geometry.WalkableArea` used in the simulation | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach to deal with the overlapping situation:
Create the walkable area (without obstacles) as shapely.Polyon
and afterward subtract each of the obstacles from that polygon:
import shapely
import pedpy
outer = shapely.box(-5, -5, 5, 5)
obstacle = shapely.box(-1, -1, 6, 1)
walkable_area_poly = outer.difference(obstacle)
walkable_area = pedpy.WalkableArea(walkable_area_poly)
That way, the obstacles could reach to the border and the results may be more accurate
_log.warning( | ||
f"The interpolated trajectory potentially deviates up to " | ||
f"{str(max_deviation)} m from the original trajectory, at least " | ||
f"for the fastest pedestrian with max. speed of {str(max_speed)} m/s. " | ||
f"If smaller deviations are required, choose a higher frame rate. " | ||
f"The current frame rate is {str(frame_rate)} fps." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: Warning vs exception
encoding="utf-8-sig", | ||
) | ||
|
||
data.rename(columns=rename_mapping, inplace=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the ruff linter: PD002 inplace=True
should be avoided; it has inconsistent behavior
list | ||
|
||
""" | ||
_supported_types = ["RECTANGLE", "POLYGON"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Vadere currently (or in the near future) support more shape types?
Y_COL: "startY-PID1", # "-PID1" see comment above | ||
} | ||
) | ||
data["simTime"] = data["simTime"] / frame_rate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be nan for the default frame_rate
(=0). If so, is the default=0 really needed?
This feature allows users to load trajectory and scenario files that originate from Vadere. The jupyter notebook
user_guide
provides an example. Data for the example and test data are added. Currently, this feature supports data created with Vadere 3.0.