Merge pull request #3957 from alphagov/make-calculating-overlaps-faster

Make calculating overlapping areas faster

app/broadcast_areas/models.py

@@ -86,6 +86,12 @@ class BroadcastArea(BaseBroadcastArea, SortableMixin):
     def __init__(self, row):
         self.id, self.name, self._count_of_phones, self.library_id = row
 
+    @classmethod
+    def from_row_with_simple_polygons(cls, row):
+        instance = cls(row[:4])
+        instance.simple_polygons = Polygons(row[4])
+        return instance
+
     @cached_property
     def polygons(self):
         return Polygons(
@@ -158,11 +164,11 @@ class CustomBroadcastArea(BaseBroadcastArea):
 
     simple_polygons = polygons
 
-    @property
+    @cached_property
     def overlapping_areas(self):
         if not self.polygons:
             return []
-        return broadcast_area_libraries.get_areas([
+        return broadcast_area_libraries.get_areas_with_simple_polygons([
             overlap.data for overlap in rtree_index.query(
                 Rect(*self.polygons.bounds)
             )
@@ -171,7 +177,7 @@ class CustomBroadcastArea(BaseBroadcastArea):
     @cached_property
     def count_of_phones(self):
         return sum(
-            area.polygons.ratio_of_intersection_with(self.polygons) * area.count_of_phones
+            area.simple_polygons.ratio_of_intersection_with(self.polygons) * area.count_of_phones
             for area in self.overlapping_areas
         )
 
@@ -221,13 +227,13 @@ class BroadcastAreaLibraries(SerialisedModelCollection, GetItemByIdMixin):
     def __init__(self):
         self.items = BroadcastAreasRepository().get_libraries()
 
-    def get_areas(self, *area_ids):
+    def get_areas(self, area_ids):
-        # allow people to call `get_areas('a', 'b') or get_areas(['a', 'b'])`
-        if len(area_ids) == 1 and isinstance(area_ids[0], list):
-            area_ids = area_ids[0]
-
         areas = BroadcastAreasRepository().get_areas(area_ids)
         return [BroadcastArea(area) for area in areas]
 
+    def get_areas_with_simple_polygons(self, area_ids):
+        areas = BroadcastAreasRepository().get_areas_with_simple_polygons(area_ids)
+        return [BroadcastArea.from_row_with_simple_polygons(area) for area in areas]
+
 
 broadcast_area_libraries = BroadcastAreaLibraries()

app/broadcast_areas/repo.py

@@ -130,7 +130,7 @@ class BroadcastAreasRepository(object):
         SELECT id, name, count_of_phones, broadcast_area_library_id
         FROM broadcast_areas
         WHERE id IN ({})
-        """.format(("?," * len(area_ids))[:-1])
+        """.format(",".join("?" * len(area_ids)))
 
         results = self.query(q, *area_ids)
 
@@ -141,6 +141,23 @@ class BroadcastAreasRepository(object):
 
         return areas
 
+    def get_areas_with_simple_polygons(self, area_ids):
+        q = """
+        SELECT broadcast_areas.id, name, count_of_phones, broadcast_area_library_id, simple_polygons
+        FROM broadcast_areas
+        JOIN broadcast_area_polygons on broadcast_area_polygons.id = broadcast_areas.id
+        WHERE broadcast_areas.id IN ({})
+        """.format(",".join("?" * len(area_ids)))
+
+        results = self.query(q, *area_ids)
+
+        areas = [
+            (row[0], row[1], row[2], row[3], json.loads(row[4]))
+            for row in results
+        ]
+
+        return areas
+
     def get_all_areas_for_library(self, library_id):
         is_multi_tier_library = self.query("""
         SELECT exists(

app/main/views/broadcast.py

@@ -287,7 +287,7 @@ def choose_broadcast_sub_area(service_id, broadcast_message_id, library_slug, ar
         broadcast_message_id,
         service_id=current_service.id,
     )
-    area = BroadcastMessage.libraries.get_areas(area_slug)[0]
+    area = BroadcastMessage.libraries.get_areas([area_slug])[0]
 
     back_link = _get_broadcast_sub_area_back_link(service_id, broadcast_message_id, library_slug)
 

app/models/broadcast_message.py

@@ -18,9 +18,9 @@ from app.notify_client.broadcast_message_api_client import (
     broadcast_message_api_client,
 )
 
-ESTIMATED_AREA_OF_LARGEST_UK_COUNTY = broadcast_area_libraries.get_areas(
+ESTIMATED_AREA_OF_LARGEST_UK_COUNTY = broadcast_area_libraries.get_areas([
     'ctyua19-E10000023'  # North Yorkshire
-)[0].polygons.estimated_area
+])[0].polygons.estimated_area
 
 
 class BroadcastMessage(JSONModel):
@@ -172,14 +172,14 @@ class BroadcastMessage(JSONModel):
     def cancelled_by(self):
         return User.from_id(self.cancelled_by_id)
 
-    @property
+    @cached_property
     def count_of_phones(self):
         return round_to_significant_figures(
             sum(area.count_of_phones for area in self.areas),
             1
         )
 
-    @property
+    @cached_property
     def count_of_phones_likely(self):
         estimated_area = self.simple_polygons.estimated_area
 
@@ -203,7 +203,7 @@ class BroadcastMessage(JSONModel):
 
     def get_areas(self, areas):
         return broadcast_area_libraries.get_areas(
-            *areas
+            areas
         )
 
     def get_simple_polygons(self, areas):

tests/app/broadcast_areas/test_broadcast_area.py

@@ -77,40 +77,25 @@ def test_loads_areas_from_libraries(id):
     assert (
         broadcast_area_libraries.get('ctry19').get(id)
     ) == (
-        broadcast_area_libraries.get_areas(id)[0]
+        broadcast_area_libraries.get_areas([id])[0]
     )
 
 
 def test_get_names_of_areas():
-    areas = broadcast_area_libraries.get_areas(
+    areas = broadcast_area_libraries.get_areas([
         'ctry19-W92000004',
         'lad20-W06000014',
         'ctry19-E92000001',
-    )
+    ])
     assert [area.name for area in sorted(areas)] == [
         'England', 'Vale of Glamorgan', 'Wales',
     ]
 
 
-def test_get_areas_accepts_lists():
-    areas_from_list = broadcast_area_libraries.get_areas(
-        [
-            'ctry19-W92000004',
-            'ctry19-E92000001',
-        ]
-    )
-    areas_from_args = broadcast_area_libraries.get_areas(
-        'ctry19-W92000004',
-        'ctry19-E92000001',
-    )
-    assert len(areas_from_args) == len(areas_from_list) == 2
-    assert areas_from_args == areas_from_list
-
-
 def test_has_polygons():
 
-    england = broadcast_area_libraries.get_areas('ctry19-E92000001')[0]
+    england = broadcast_area_libraries.get_areas(['ctry19-E92000001'])[0]
-    scotland = broadcast_area_libraries.get_areas('ctry19-S92000003')[0]
+    scotland = broadcast_area_libraries.get_areas(['ctry19-S92000003'])[0]
 
     assert len(england.polygons) == 35
     assert len(scotland.polygons) == 195
@@ -130,7 +115,7 @@ def test_polygons_are_enclosed():
 
 def test_lat_long_order():
 
-    england = broadcast_area_libraries.get_areas('ctry19-E92000001')[0]
+    england = broadcast_area_libraries.get_areas(['ctry19-E92000001'])[0]
 
     lat_long = england.polygons.as_coordinate_pairs_lat_long
     long_lat = england.polygons.as_coordinate_pairs_long_lat
@@ -212,13 +197,13 @@ def test_every_area_has_count_of_phones(library):
 
 ))
 def test_count_of_phones_for_all_levels(area_id, area_name, expected_count):
-    area = broadcast_area_libraries.get_areas(area_id)[0]
+    area = broadcast_area_libraries.get_areas([area_id])[0]
     assert area.name == area_name
     assert area.count_of_phones == expected_count
 
 
 def test_city_of_london_counts_are_not_derived_from_population():
-    city_of_london = broadcast_area_libraries.get_areas('lad20-E09000001')[0]
+    city_of_london = broadcast_area_libraries.get_areas(['lad20-E09000001'])[0]
 
     assert city_of_london.name == 'City of London'
     assert len(city_of_london.sub_areas) == len(CITY_OF_LONDON.WARDS) == 25
@@ -315,7 +300,7 @@ def test_phone_density(
     area, expected_phones_per_square_mile,
 ):
     assert close_enough(
-        broadcast_area_libraries.get_areas(area)[0].phone_density,
+        broadcast_area_libraries.get_areas([area])[0].phone_density,
         expected_phones_per_square_mile,
     )
 
@@ -351,11 +336,11 @@ def test_estimated_bleed(
     area, expected_bleed_in_m, expected_bleed_in_degrees,
 ):
     assert close_enough(
-        broadcast_area_libraries.get_areas(area)[0].estimated_bleed_in_m,
+        broadcast_area_libraries.get_areas([area])[0].estimated_bleed_in_m,
         expected_bleed_in_m,
     )
     assert close_enough(
-        broadcast_area_libraries.get_areas(area)[0].estimated_bleed_in_degrees,
+        broadcast_area_libraries.get_areas([area])[0].estimated_bleed_in_degrees,
         expected_bleed_in_degrees,
     )
 
@@ -377,7 +362,7 @@ def test_estimated_bleed(
             'Stoke Bishop',
             'Windmill Hill',
         ],
-        73_496,
+        73_119,
     ),
     (
         SKYE,
@@ -387,7 +372,7 @@ def test_estimated_bleed(
             'Na Hearadh agus Ceann a Deas nan Loch',
             'Wester Ross, Strathpeffer and Lochalsh',
         ],
-        3_517,
+        3_534,
     ),
 ))
 def test_count_of_phones_for_custom_area(