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

Continuity equation and fundamental diagram of pedestrians #242

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

Conversation

ChristianHirt
Copy link
Contributor

@ChristianHirt ChristianHirt commented Nov 7, 2023

This PR implements the paper
Adrian et al, 2024, Continuity equation and fundamental diagram of pedestrians

  • Add Notebook explaining new methods.
  • Add methods to compute species, line_speed, line_density, line_flow.
  • Add methods to plot speed, density and flow for both species and the total value.
  • Add tests to verify new calculations

ChristianHirt and others added 30 commits October 16, 2023 13:17
calculate speed in movement direction using compute_individual_speed,
use tuple instead dict for tests
chraibi
chraibi previously approved these changes Dec 27, 2024
during the addition of line quantities, especially when there is only
one direction, this is often the case.
- use cdot instead of * to refer to multiplication
- use proper intersection symbol
- flow formula had an error (should devise by A (area) not V)
- For compute_line_speed
- Add tests
@chraibi chraibi added speed Anything related to computing the speed density Anything related to computing the density science Relevant for scientific publications (e.g., enabling citing) labels Dec 30, 2024
@@ -313,6 +314,16 @@ def line(self):
"""
return self._line

def normal_vector(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return type missing. Thought this would be caught be some linter or mypy...

@@ -313,6 +314,16 @@ def line(self):
"""
return self._line

def normal_vector(self):
"""Returns a normalized normal vector of the line."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring not complete, should als contain a description of the return value, e.g.,

        """Measurement line as :class:`shapely.LineString`.
        Returns:
            Measurement line as :class:`shapely.LineString`.
        """

individual_speed: pd.DataFrame,
species: pd.DataFrame,
) -> None:
"""Centralize input validation with clear error messages."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to explain what is checked here in the docstring.

individual_speed: pd.DataFrame,
species: pd.DataFrame,
) -> None:
"""Centralize input validation with clear error messages."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, a little more information in the docstring would be nice

Comment on lines 1149 to 1152
"""Applies lambda for both species for frames where Polygon intersects line.

lambda_for_group is called with a group containing
the data of one species and a Measurement Line.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A proper docstring would be nice here too, this is used at multiple places and may be reused by other methods as well

.. list-table::
:widths: 20 80

* - :doc:`CECM <../CECM>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the same heading as in the notebooks: Fundamental Diagrams at Measurement Lines. Then it is consistent and more understandable. CECM was a working name, not sure if somebody knows what it stands for.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename the file to match its title: fundamental_diagrams_at_measurement_lines. Also update the link in the docs

"j^S(t) = \\sum_{i \\in S} m_i \\cdot v_i(t) \\cdot n_{l} \\cdot \\frac{1}{A_i(t)} \\cdot \\frac{w_i(t)}{w}\n",
"$$\n",
"\n",
"### Explanation of Symbols\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it on purpose, that this is a subsection of "Compute Line flow"?

:widths: 20 80

* - :doc:`CECM <../CECM>`
- calculating speed, flow and density when analysing pedestrian trajectories at a measurement line.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: mention here, that this a Continuity Equation conform way

@chraibi
Copy link
Collaborator

chraibi commented Jan 13, 2025

@schroedtert I think I fixed all stuff regarding your review.
@ThoChat You can have a look now. Please focus only on the mathematical implementation of the function compared to the paper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
density Anything related to computing the density science Relevant for scientific publications (e.g., enabling citing) speed Anything related to computing the speed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants