287 improve osm maps with tag metadata #294

r-leyshon · 2024-06-19T11:19:07Z

Description

FindTags now issues a PerformanceWarning if the OSM file it is being instantiated with is larger than 500 KB.
FindLocations.plot_ids() now has an include_tags parameter, False by default. Setting this to True adds all the tag metadata to the folium map tooltip.
FindLocations.plot_ids() now exposes selected folium map styling parameters.
An example implementation is included in the quarto OSM tutorial.
Finally, all the runners broke when raising PR, as breaking changes with numpy 2.0.0 that had nothing to do with this diff. My solution has been to pin numpy version to that which passed in the previous successful run.

Fixes #287

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Test configuration details:

OS: macos
Python version: 3.9.13
Java version: openjdk 11.0.19 2023-04-18 LTS
Python management system: pip

Advice for reviewer

Checklist:

My code follows the intended structure of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Additional comments

…h large osm fixture

CBROWN-ONS

Great stuff Rich. Just a few small comments on this

CBROWN-ONS · 2024-06-19T12:51:10Z

src/transport_performance/osm/validate_osm.py

+        style_kwds: dict = {
+            "color": "#3f5277",
+            "fill": True,
+            "fillOpacity": 0.3,
+            "fillColor": "#3f5277",
+            "weight": 4,
+        },


is this the correct approach? it assumes that a user wants to specify all of these features.
In theory, a user might only want to change e.g., the colour, however to do this they would have to specify all the other keywords themselves since style_kwds would be overwritten.

I think this one is ok:

CBROWN-ONS · 2024-06-19T12:52:31Z

src/transport_performance/osm/validate_osm.py

@@ -719,6 +873,7 @@ def plot_ids(
        _type_defence(ids, "ids", list)
        _type_defence(feature_type, "feature_type", str)
        _type_defence(crs, "crs", (str, int))
+        _type_defence(include_tags, "include_tags", bool)


Can you also add defences for the remainder of the new params?

agree. good shout.

CBROWN-ONS · 2024-06-19T12:54:42Z

src/transport_performance/osm/validate_osm.py

@@ -37,6 +39,12 @@
 # ---------utilities-----------


+class PerformanceWarning(UserWarning):


Do you think inheriting from Warning would be better than UserWarning here?

CBROWN-ONS · 2024-06-19T12:56:42Z

src/transport_performance/osm/validate_osm.py

@@ -541,6 +549,16 @@ def __init__(
        _is_expected_filetype(
            osm_pth, "osm_pth", check_existing=True, exp_ext=".pbf"
        )
+        self.large_file_thresh = 50000  # 50 KB


Very small thing that doesn't really matter.
Do you think this needs to be defined as an attribute of the class? This should be a one time use in the initial size check therefore it can simply be assigned to a local variable.
The only downfall of having it as a class attribute is that a user could potentially view it as something they can alter.

agree with your point. I used name mangling and updated the docstring.

CBROWN-ONS · 2024-06-19T12:58:38Z

src/transport_performance/osm/validate_osm.py

+        osm_size = os.path.getsize(osm_pth)
+        if osm_size > self.large_file_thresh:
+            warnings.warn(
+                f"PBF file is {osm_size} bytes. Tag operations are expensive."
+                " Consider filtering the pbf file smaller than"
+                f" {self.large_file_thresh} bytes",
+                PerformanceWarning,
+            )


A very small technicality, but the error message provokes the user to reduce file size < 50000, however an error isn't raised if file size is 50000.

It'll still work for sizes greater than 50 KB, it just gets slow on init. It's hoped this would nudge the user to think about the size of the files prior to using FindTags as it's the slowest of all the osm flavoured classes.

CBROWN-ONS · 2024-06-20T05:37:15Z

src/transport_performance/osm/validate_osm.py

+        for d in [dict1, dict2]:
+            if not isinstance(d, dict):
+                raise TypeError(f"Expected dict but found {type(d)}: {d}")
+        for id_, tags in dict1.items():  # child_tags is nested
+            # find duplicated keys and prepend parent keys
+            if dupes := set(tags.keys()).intersection(dict2.keys()):
+                for key in dupes:
+                    dict2[f"{prepend_pattern}{key}"] = dict2.pop(key)
+            # merge parent and child tag collections
+            tags_out[id_] = tags | dict2
+        return tags_out


A very good way of doing this.

It's neat now, but it took a lot of wiggling to get there!

CBROWN-ONS · 2024-06-20T06:54:23Z

docs/tutorials/osm/index.qmd

@@ -353,6 +353,14 @@ To read more on `osmosis` filtering strategies, refer to the `completeWays` and
 `completeRelations` flag descriptions in the
 [Osmosis detailed usage documentation](https://wiki.openstreetmap.org/wiki/Osmosis/Detailed_Usage_0.48).

+
+Note that additional metadata can be added to the map by setting `include_tags=True`. Adding this rich contextual data to the map can be useful but is also expensive. This operation should be avoided for large osm files, for example anything over 500 KB.


'computationally expensive' rather than 'expensive' here would be a better description, in my opinion, as it provides greater context to the reader.

…bute

r-leyshon · 2024-06-25T09:24:22Z

Hey @CBROWN-ONS , I've just implemented your suggestions and pushed the diff to the PR. Thanks for your review on this, some good catches in there.

codecov-commenter · 2024-06-25T09:29:53Z

Codecov Report

Attention: Patch coverage is 98.24561% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.13%. Comparing base (3fa154e) to head (3004a81).

Files	Patch %	Lines
src/transport_performance/osm/validate_osm.py	98.24%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #294   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files          21       21           
  Lines        1927     1983   +56     
=======================================
+ Hits         1891     1946   +55     
- Misses         36       37    +1

Flag	Coverage Δ
unittests	`98.13% <98.24%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CBROWN-ONS · 2024-06-25T09:52:17Z

Thanks for the changes @r-leyshon . All good, will merge this now.

r-leyshon added 15 commits June 5, 2024 13:27

feat: OSM filtering operations

40cd612

feat: Visualise OSM Way feature

ddb204f

feat: Finalised version of OSM tutorial

d9dcaad

feat: Updated dependencies in GTFS tutorial

bf4ac93

feat: Way feature tag metadata added to folium maps

640e4c1

feat: Add node context to folium map

d297281

test: Folium maps produced when specify plot_ids with include_tags=True

88a98f6

refactor: Move utility to class method

ccaa46f

refactor: Remove surplus dict copy statement

58b157a

docs: Update osm tutorial with example adding tag metadata to folium map

78dd322

test: Tooltip has been assembled as needed

342e467

test: Internal method raises type error

0d37238

refactor: Need to import PerformanceWarning to test module

ed563fe

test: runexpensive test for raise of perf warning on FindTag init wit…

177401f

…h large osm fixture

chore: Bring up to speed with changes in dev

0754155

r-leyshon added documentation Improvements or additions to documentation enhancement New feature or request OSM labels Jun 19, 2024

r-leyshon added this to the Version 1 Release milestone Jun 19, 2024

r-leyshon requested a review from CBROWN-ONS June 19, 2024 11:19

r-leyshon linked an issue Jun 19, 2024 that may be closed by this pull request

improve OSM maps with tag metadata #287

Closed

chore: Pin numpy to avoid numpy.dtype size change error with numpy=2.0.0

0533e96

CBROWN-ONS self-assigned this Jun 19, 2024

CBROWN-ONS requested changes Jun 20, 2024

View reviewed changes

CBROWN-ONS reviewed Jun 20, 2024

View reviewed changes

r-leyshon added 5 commits June 25, 2024 09:48

docs: PR docs suggestion

06f2f70

refactor: PR make large_file_thresh more obvious as an internal attri…

6aed5d4

…bute

refactor: PR inherit from Warning not UserWarning

5a1ccea

feat: PR type defences for additional params

7a84776

chore: Consistency in req file

3004a81

r-leyshon force-pushed the 287-improve-osm-maps-with-tag-metadata branch from f3491a1 to 3004a81 Compare June 25, 2024 09:23

CBROWN-ONS approved these changes Jun 25, 2024

View reviewed changes

CBROWN-ONS merged commit cef299b into dev Jun 25, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

287 improve osm maps with tag metadata #294

287 improve osm maps with tag metadata #294

r-leyshon commented Jun 19, 2024 •

edited

Loading

CBROWN-ONS left a comment

CBROWN-ONS Jun 19, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 19, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 19, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 19, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 19, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 20, 2024

r-leyshon Jun 25, 2024

CBROWN-ONS Jun 20, 2024

r-leyshon Jun 25, 2024

r-leyshon commented Jun 25, 2024

codecov-commenter commented Jun 25, 2024 •

edited

Loading

CBROWN-ONS commented Jun 25, 2024

		@@ -37,6 +39,12 @@
		# ---------utilities-----------


		class PerformanceWarning(UserWarning):

287 improve osm maps with tag metadata #294

287 improve osm maps with tag metadata #294

Conversation

r-leyshon commented Jun 19, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Advice for reviewer

Checklist:

Additional comments

CBROWN-ONS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-leyshon commented Jun 25, 2024

codecov-commenter commented Jun 25, 2024 • edited Loading

Codecov Report

CBROWN-ONS commented Jun 25, 2024

r-leyshon commented Jun 19, 2024 •

edited

Loading

codecov-commenter commented Jun 25, 2024 •

edited

Loading