Conversation
pedpy/plotting/plotting.py
Outdated
| cb_label = kwargs.pop("cb_label", None) | ||
| color_by_column = kwargs.pop("color_by_column", None) | ||
| voronoi_colormap = plt.get_cmap(kwargs.pop("cmap", "YlGn")) | ||
| voronoi_colormap = plt.get_cmap(kwargs.pop("cmap", "YlGng")) |
Nan: not visited at all 0: visited at t=0
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
schroedtert
left a comment
schroedtert
left a comment
There was a problem hiding this comment.
Only some minor things, but overall it looks great!
| if not isinstance(traj_data, TrajectoryData): | ||
| raise PedPyTypeError(f"`traj_data` must be an instance of TrajectoryData, got {type(traj_data).__name__}.") |
There was a problem hiding this comment.
We don't check this anywhere else, not sure if we should add a test here. Otherwise, we would also have to check the other parameters. I think use type hints should be enough, as the dynamic types are a Python property
| def test_plot_rset_map_preserves_title_and_labels(): | ||
| """Title and axis labels set by plot_rset_map must survive the | ||
| internal call to plot_walkable_area.""" | ||
| walkable_area = WalkableArea(shapely.box(0, 0, 2, 2)) | ||
| rset_map = np.array([[np.nan, 1.0], [2.0, 3.0]]) | ||
|
|
||
| fig, ax = plt.subplots() | ||
| returned_ax = plot_rset_map( | ||
| walkable_area=walkable_area, | ||
| rset_map=rset_map, | ||
| axes=ax, | ||
| title="Custom Title", | ||
| ) | ||
|
|
||
| assert returned_ax.get_title() == "Custom Title" | ||
| assert "x" in returned_ax.get_xlabel().lower() | ||
| assert "y" in returned_ax.get_ylabel().lower() | ||
| plt.close(fig) |
There was a problem hiding this comment.
This should be moved to a plotting unit test file as here not the rset functionality but the plotting is tested. Currently, plotting is not tested at all, we should discuss, if this is something we want and need
| from pedpy.methods.profile_calculator import RsetMethod, compute_rset_map | ||
| from pedpy.plotting.plotting import plot_rset_map | ||
|
|
||
| matplotlib.use("Agg") |
There was a problem hiding this comment.
What does this import do?
| def _make_traj( | ||
| ids: list[int], | ||
| frames: list[int], | ||
| xs: list[float], | ||
| ys: list[float], | ||
| frame_rate: float = 1.0, | ||
| ) -> TrajectoryData: | ||
| df = pd.DataFrame({"id": ids, "frame": frames, "x": xs, "y": ys}) | ||
| return TrajectoryData(data=df, frame_rate=frame_rate) |
There was a problem hiding this comment.
This is something which can be moved to tests/utils as it can be reused by other tests, see https://github.com/PedestrianDynamics/PedPy/pull/515/changes#diff-fa021950f52f334d684b781cb14faca79bcf9802d1bb292d732377eb99ae5bc4R33-R44
| ")\n", | ||
| "plt.show()" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Please add hide-input as cell tag (as the other plotting cells)
| " walkable_area=walkable_area,\n", | ||
| " rset_map=rset_min,\n", | ||
| " axes=ax1,\n", | ||
| " title=\"RSET (min)\",\n", |
There was a problem hiding this comment.
Can you also add the time here too? As for RSET with max
| " walkable_area=walkable_area,\n", | ||
| " rset_map=rset_mean,\n", | ||
| " axes=ax2,\n", | ||
| " title=\"RSET (mean)\",\n", |
There was a problem hiding this comment.
Can you also add the time here too? As for RSET with max
| "- **MIN**: earliest time any pedestrian was observed in each cell\n", | ||
| "- **MEAN**: average time pedestrians were observed in each cell\n", | ||
| "\n", | ||
| "> **Note:** Cells with no trajectory data contain *NaN*. Use `np.nanmax`, `np.nanmin`, etc. instead of `np.max`/`np.min` when computing summary statistics from the returned array.\n" |
There was a problem hiding this comment.
Better use:
:::{note}
Cells with no trajectory data contain *NaN*. Use `np.nanmax`, `np.nanmin`, etc. instead of `np.max`/`np.min` when computing summary statistics from the returned array
:::
This will create something like this:
2 participants
closes #508