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.

Sign up for GitHub

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

Jump to bottom

[Jormungandr] Manage address within zones in journeys #4071

Merged
merged 8 commits into from
Jul 19, 2023
Merged

[Jormungandr] Manage address within zones in journeys #4071

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
583c392
address to poi
Jul 17, 2023
07433f8
tests added
Jul 17, 2023
683f094
unused func
Jul 17, 2023
8a2960a
tests added
Jul 17, 2023
322a41a
corrections
Jul 17, 2023
b327f9a
Fix tests
Jul 18, 2023
0ed08a5
corrections
Jul 19, 2023
1bb0a1d
refactoring
Jul 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions source/jormungandr/jormungandr/scenarios/new_default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
get_pt_object_from_json,
json_address_from_uri,
is_olympic_site,
transform_entrypoint,
)
from jormungandr.error import generate_error
from jormungandr.utils import Coords
Expand Down Expand Up @@ -1750,14 +1751,17 @@ def get_entrypoint_detail(self, entrypoint, instance, request_id):
)

if detail:
return detail
# Transform address in (from, to) to poi with ES
return transform_entrypoint(detail, entrypoint)

if not isinstance(instance.autocomplete, GeocodeJson):
bragi = global_autocomplete.get(app.config.get('DEFAULT_AUTOCOMPLETE_BACKEND', 'bragi'))
if bragi:
# if the instance's autocomplete is not a geocodejson autocomplete, we also check in the
# global autocomplete instance
return bragi.get_object_by_uri(entrypoint, instances=[instance], request_id=request_id)
detail = bragi.get_object_by_uri(entrypoint, instances=[instance], request_id=request_id)
# Transform address in (from, to) to poi with ES
return transform_entrypoint(detail, entrypoint)

return None

Expand Down
131 changes: 130 additions & 1 deletion source/jormungandr/jormungandr/tests/utils_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@
read_best_boarding_positions,
read_origin_destination_data,
make_origin_destination_key,
portable_min,
get_last_pt_section,
get_first_pt_section,
transform_entrypoint,
)
import pytz
from jormungandr import app
Expand Down Expand Up @@ -403,3 +403,132 @@ def test_get_olympic_site_pt_section_journey_only_street_network_section():
last_section = get_last_pt_section(pb_j)
assert first_section.duration == 1
assert last_section.duration == 3


def test_transform_entrypoint_none_value():
res = transform_entrypoint(None, "123")
assert res is None


def test_transform_entrypoint_embedded_type_not_address():
stop_area = {
"id": "stop_area:123",
"name": "Stop area (Paris)",
"quality": 0,
"embedded_type": "stop_area",
"stop_area": {
"id": "stop_area:123",
"coord": {"lat": "48.845971", "lon": "2.339872"},
"label": "Stop area (Paris)",
"name": "Stop area",
},
}
res = transform_entrypoint(stop_area, stop_area["id"])
assert res["id"] == stop_area["id"]
assert res["name"] == stop_area["name"]
assert res["embedded_type"] == stop_area["embedded_type"]
assert res["stop_area"]["id"] == stop_area["stop_area"]["id"]
assert res["stop_area"]["label"] == stop_area["stop_area"]["label"]
assert res["stop_area"]["name"] == stop_area["stop_area"]["name"]
assert res["stop_area"]["coord"]["lat"] == stop_area["stop_area"]["coord"]["lat"]
assert res["stop_area"]["coord"]["lon"] == stop_area["stop_area"]["coord"]["lon"]


def test_without_within_zones():
address = {
"id": "2.339872;48.845971",
"name": "58BIS Boulevard Saint-Michel (Paris)",
"quality": 0,
"embedded_type": "address",
"address": {
"id": "2.339872;48.845971",
"coord": {"lat": "48.845971", "lon": "2.339872"},
"house_number": 58,
"label": "58BIS Boulevard Saint-Michel (Paris)",
"name": "58BIS Boulevard Saint-Michel",
},
}
res = transform_entrypoint(address, address["id"])
assert res["id"] == address["id"]
assert res["name"] == address["name"]
assert res["embedded_type"] == address["embedded_type"]
assert res["address"]["id"] == address["address"]["id"]
assert res["address"]["label"] == address["address"]["label"]
assert res["address"]["name"] == address["address"]["name"]
assert res["address"]["coord"]["lat"] == address["address"]["coord"]["lat"]
assert res["address"]["coord"]["lon"] == address["address"]["coord"]["lon"]


def test_with_empty_within_zones():
address = {
"id": "2.339872;48.845971",
"name": "58BIS Boulevard Saint-Michel (Paris)",
"quality": 0,
"embedded_type": "address",
"address": {
"id": "2.339872;48.845971",
"coord": {"lat": "48.845971", "lon": "2.339872"},
"house_number": 58,
"label": "58BIS Boulevard Saint-Michel (Paris)",
"name": "58BIS Boulevard Saint-Michel",
},
"within_zones": [],
}
res = transform_entrypoint(address, address["id"])
assert res["id"] == address["id"]
assert res["name"] == address["name"]
assert res["embedded_type"] == address["embedded_type"]
assert res["address"]["id"] == address["address"]["id"]
assert res["address"]["label"] == address["address"]["label"]
assert res["address"]["name"] == address["address"]["name"]
assert res["address"]["coord"]["lat"] == address["address"]["coord"]["lat"]
assert res["address"]["coord"]["lon"] == address["address"]["coord"]["lon"]


def test_address_with_within_zones():
poi = {
"id": "poi:10309",
"name": "Jardin du Luxembourg (Paris)",
"quality": 0,
"embedded_type": "poi",
"poi": {
"id": "poi:10309",
"coord": {"lat": "48.8487134929", "lon": "2.33642846001"},
"label": "Jardin du Luxembourg (Paris)",
"name": "Jardin du Luxembourg",
"poi_type": {"id": "poi_type:jardin", "name": "Jardin"},
"children": [
{
"id": "poi:osm:node:24908972",
"name": "Porte Saint-Michel",
"label": "Porte Saint-Michel (Paris)",
"coord": {"lon": "2.340268", "lat": "48.8468553"},
"type": "poi",
"poi_type": {"id": "poi_type:access_point", "name": "Point d'accès"},
}
],
},
}
address = {
"id": "2.339872;48.845971",
"name": "58BIS Boulevard Saint-Michel (Paris)",
"quality": 0,
"embedded_type": "address",
"address": {
"id": "2.339872;48.845971",
"coord": {"lat": "48.845971", "lon": "2.339872"},
"house_number": 58,
"label": "58BIS Boulevard Saint-Michel (Paris)",
"name": "58BIS Boulevard Saint-Michel",
},
"within_zones": [poi],
}
res = transform_entrypoint(address, address["id"])
assert res["id"] == poi["id"]
assert res["name"] == poi["name"]
assert res["embedded_type"] == poi["embedded_type"]
assert res["poi"]["id"] == poi["poi"]["id"]
assert res["poi"]["label"] == poi["poi"]["label"]
assert res["poi"]["name"] == poi["poi"]["name"]
assert res["poi"]["coord"]["lat"] == address["address"]["coord"]["lat"]
assert res["poi"]["coord"]["lon"] == address["address"]["coord"]["lon"]
24 changes: 24 additions & 0 deletions source/jormungandr/jormungandr/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,30 @@ def get_pt_object_from_json(dict_pt_object, instance):
return pt_object


def transform_entrypoint(dict_pt_object, uri):
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find a good name for this function... but I'd say we should try to find another name. Here are suggestions for brainstorming, maybe you will find other ideas?

Building the name of the function with this shape.
<verb>_<name>

<verbs>:

  • improve
  • customize
  • replace
  • rectify

<names>:

  • entrypoint_detail
  • address (replace_address for example)

other ideas:

  • rectify_entrypoint_with_uri
  • entrypoint_uri_refocus (I kind of like this one, we're changing back the entrypoint to refocus on the end-user information: the from or the to)

None of them are really good, but I'm hoping to help find ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if not dict_pt_object:
return None
if MAP_STRING_PTOBJECT_TYPE.get(dict_pt_object.get("embedded_type")) != type_pb2.ADDRESS:
return dict_pt_object
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if you should have a second sub-function here. The goal of a sub-function is for documentation. Since the sub-function also has a name, it helps to understand a sub-part of the code.

Suggested change
if MAP_STRING_PTOBJECT_TYPE.get(dict_pt_object.get("embedded_type")) != type_pb2.ADDRESS:
return dict_pt_object
if MAP_STRING_PTOBJECT_TYPE.get(dict_pt_object.get("embedded_type")) == type_pb2.ADDRESS:
replace_address_with_custom_poi(dict_pt_object, uri)
else:
return dict_pt_object

All the code below would go in the new function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

first_within_zone = next(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could rename this new_poi to express better the fact that we're actually creating a new object (based on an existing POI, but with custom coordinates).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

(
wz
for wz in dict_pt_object.get("within_zones", [])
if MAP_STRING_PTOBJECT_TYPE.get(wz.get("embedded_type")) == type_pb2.POI
and wz.get("poi", {}).get("children", [])
),
None,
)
if not first_within_zone:
return dict_pt_object
if not is_coord(uri):
return dict_pt_object
lon, lat = get_lon_lat(uri)
first_within_zone["poi"]["coord"]["lon"] = "{}".format(lon)
first_within_zone["poi"]["coord"]["lat"] = "{}".format(lat)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put a comment here to express why we're changing the coordinate. Here is a suggestion.

# We're putting back the coordinate of the end-user, who is in a POI area (with entrypoints).
# If we don't, barycenter of the POI area will be displayed which means nothing for the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done

return first_within_zone


def json_address_from_uri(uri):
if is_coord(uri):
lon, lat = get_lon_lat(uri)
Expand Down
Loading