Skip to content
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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

gitsimonr
Copy link

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.

Simon Rahn added 23 commits July 26, 2024 14:13
…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
@schroedtert
Copy link
Collaborator

Thanks for your contribution, we are really looking forward to support Vadere trajectory files!
I will try to take a look at your work next week, unfortunately I won't have the time before.

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

Copy link
Collaborator

@schroedtert schroedtert left a 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,
Copy link
Collaborator

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?

Comment on lines +667 to +669
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.
Copy link
Collaborator

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?

Comment on lines +848 to +851
_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."
Copy link
Collaborator

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

Comment on lines +930 to +954
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
"""
Copy link
Collaborator

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

Comment on lines +921 to +927
_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."
)
Copy link
Collaborator

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)
Copy link
Collaborator

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"]
Copy link
Collaborator

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
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants