Compute neighbor distance #392
Conversation
schroedtert
force-pushed
the
neighbors-distance
branch
2 times, most recently
from
2082fe9 to
5dfdd1a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
schroedtert
force-pushed
the
neighbors-distance
branch
from
5dfdd1a to
de0963a
Compare
pedpy/methods/method_utils.py
Outdated
| as_list (bool): Return the neighbors as a list per pedestrian and frame, | ||
| if true, otherwise each neighbor is in a single row. |
There was a problem hiding this comment.
I added a warning in the API description and the user guide notebook that discourages usage of the function with the default value. Do we additionally want to emit a deprecation warning if it is set to True?
There was a problem hiding this comment.
After thinking about: I also added a deprecation warning, this way everybody gets informed when using the function, without any need to read a up-to-date documentation. This way more user will update their scripts (hopefully).
There was a problem hiding this comment.
I like this explicit solution better.
schroedtert
force-pushed
the
neighbors-distance
branch
3 times, most recently
from
6f86a73 to
3c8bbaa
Compare
Also add the (preferable) option to give the neighbors not as a list, but as rows. This way they can later better be used in other computations.
schroedtert
force-pushed
the
neighbors-distance
branch
from
3c8bbaa to
43c290b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
☔ View full report in Codecov by Sentry. |
| if as_list: | ||
| warnings.warn( | ||
| "The parameter 'as_list=True' is deprecated and may change in a " | ||
| "future version. It is kept for backwards compatability. We " |
| stacklevel=2, # Makes the warning appear at the caller level | ||
| ) | ||
|
|
||
| if as_list: |
There was a problem hiding this comment.
Maybe use if as_list: once instead of twice?
| warnings.warn( | ||
| "The parameter 'as_list=True' is deprecated and may change in a " | ||
| "future version. It is kept for backwards compatability. We " | ||
| "highly discourage in using this, as its result is harder to be " |
There was a problem hiding this comment.
highly discourage in using this.
| """ | ||
| if NEIGHBORS_COL in neighborhood.columns: | ||
| raise ValueError( | ||
| "For compute the distance between neighbors compute the " |
There was a problem hiding this comment.
raise ValueError(
"Cannot compute distance between neighbors with list-format data. "
"Please use the result of compute_neighbors with parameter as_list=False."
)
|
|
||
| neighbors_with_position[DISTANCE_COL] = shapely.distance( | ||
| neighbors_with_position[POINT_COL], | ||
| neighbors_with_position["point_neighbor"], |
There was a problem hiding this comment.
Can "point_neighbor" depend on POINT_COL instead of being hard-coded?
What about
NEIGHBOR_POINT_COL = POINT_COL + "_neighbor"
and then use
neighbors_with_position[DISTANCE_COL] = shapely.distance(
neighbors_with_position[POINT_COL],
neighbors_with_position[NEIGHBOR_POINT_COL],
)
| traj_data.data[[ID_COL, FRAME_COL, POINT_COL]], | ||
| left_on=[NEIGHBOR_ID_COL, FRAME_COL], | ||
| right_on=[ID_COL, FRAME_COL], | ||
| suffixes=("", "_neighbor"), |
There was a problem hiding this comment.
This is correct I assume, but does the result have a duplicate column ID_COL?
If so, maybe you can drop it:
Closes #246