From 5a20e8d5aa53d6d031ecb850ca3bccd0ce508434 Mon Sep 17 00:00:00 2001 From: napakalas Date: Tue, 26 Aug 2025 21:42:10 +1200 Subject: [PATCH 1/6] Replace feature in __features_with_id when a new feature is created due to .group element containment (#160). --- mapmaker/flatmap/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mapmaker/flatmap/__init__.py b/mapmaker/flatmap/__init__.py index 7424a482..7fa73bcc 100644 --- a/mapmaker/flatmap/__init__.py +++ b/mapmaker/flatmap/__init__.py @@ -296,9 +296,10 @@ def new_feature(self, layer_id: str, geometry, properties, is_group=False) -> Fe feature = Feature(self.__last_geojson_id, geometry, properties, is_group=is_group) self.__features_by_geojson_id[feature.geojson_id] = feature if feature.id: - if feature.id in self.__features_with_id: - pass - else: + if ( + feature.id not in self.__features_with_id + or (feature.get_property('group') and feature.get_property('interior')) + ): self.__features_with_id[feature.id] = feature if self.map_kind == MAP_KIND.FUNCTIONAL: if (name := properties.get('name', '')) != '': From 4dad361b9cce9aab7056294fbed59fa3355f2f63 Mon Sep 17 00:00:00 2001 From: napakalas Date: Thu, 28 Aug 2025 11:43:31 +1200 Subject: [PATCH 2/6] Add option to update FlatMap.__features_with_id when calling new_feature() (#160) --- mapmaker/flatmap/__init__.py | 9 +++------ mapmaker/flatmap/layers.py | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/mapmaker/flatmap/__init__.py b/mapmaker/flatmap/__init__.py index 7fa73bcc..f0915695 100644 --- a/mapmaker/flatmap/__init__.py +++ b/mapmaker/flatmap/__init__.py @@ -288,18 +288,15 @@ def get_feature_by_geojson_id(self, geojson_id: int) -> Optional[Feature]: #========================================================================= return self.__features_by_geojson_id.get(geojson_id) - def new_feature(self, layer_id: str, geometry, properties, is_group=False) -> Feature: - #===================================================================================== + def new_feature(self, layer_id: str, geometry, properties, is_group=False, update_feature_with_id=False) -> Feature: + #====================================================================================================================== self.__last_geojson_id += 1 properties['layer'] = layer_id self.properties_store.update_properties(properties) # Update from JSON properties file feature = Feature(self.__last_geojson_id, geometry, properties, is_group=is_group) self.__features_by_geojson_id[feature.geojson_id] = feature if feature.id: - if ( - feature.id not in self.__features_with_id - or (feature.get_property('group') and feature.get_property('interior')) - ): + if update_feature_with_id or feature.id not in self.__features_with_id: self.__features_with_id[feature.id] = feature if self.map_kind == MAP_KIND.FUNCTIONAL: if (name := properties.get('name', '')) != '': diff --git a/mapmaker/flatmap/layers.py b/mapmaker/flatmap/layers.py index 93960bf3..a4f5fff0 100644 --- a/mapmaker/flatmap/layers.py +++ b/mapmaker/flatmap/layers.py @@ -402,7 +402,7 @@ def add_group_features(self, group_name: str, features: list[Feature], region_properties = base_properties.copy() for region in filter(lambda p: prepared_polygon.contains(p.geometry), regions): region_properties.update(region.properties) - layer_features.append(self.flatmap.new_feature(self.id, polygon, region_properties)) + layer_features.append(self.flatmap.new_feature(self.id, polygon, region_properties, update_feature_with_id=True)) break else: for feature in features: @@ -442,7 +442,7 @@ def add_group_features(self, group_name: str, features: list[Feature], feature_group = self.flatmap.new_feature( self.id, shapely.MultiPolygon(grouped_polygons).buffer(0), - grouped_properties, is_group=True) + grouped_properties, is_group=True, update_feature_with_id=True) layer_features.append(feature_group) # So that any grouped lines don't have a duplicate id grouped_properties.pop('id', None) @@ -459,7 +459,7 @@ def add_group_features(self, group_name: str, features: list[Feature], feature_group = self.flatmap.new_feature( self.id, shapely.MultiLineString(grouped_lines), - grouped_properties, is_group=True) + grouped_properties, is_group=True, update_feature_with_id=True) layer_features.append(feature_group) # Feature specific properties have precedence over group's From cc0bf544ba043782872099aaa52804cc589e34d3 Mon Sep 17 00:00:00 2001 From: napakalas Date: Thu, 28 Aug 2025 11:53:43 +1200 Subject: [PATCH 3/6] simple tidy up (#160) --- mapmaker/flatmap/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mapmaker/flatmap/__init__.py b/mapmaker/flatmap/__init__.py index f0915695..fadafbd4 100644 --- a/mapmaker/flatmap/__init__.py +++ b/mapmaker/flatmap/__init__.py @@ -289,7 +289,7 @@ def get_feature_by_geojson_id(self, geojson_id: int) -> Optional[Feature]: return self.__features_by_geojson_id.get(geojson_id) def new_feature(self, layer_id: str, geometry, properties, is_group=False, update_feature_with_id=False) -> Feature: - #====================================================================================================================== + #=================================================================================================================== self.__last_geojson_id += 1 properties['layer'] = layer_id self.properties_store.update_properties(properties) # Update from JSON properties file From b0a900356b5904f847d1557c99785c9de03efdfe Mon Sep 17 00:00:00 2001 From: David Brooks Date: Thu, 28 Aug 2025 13:50:44 +1200 Subject: [PATCH 4/6] Detect duplicate feature ids and make sure we don't inadvertently create duplicates (#160). --- mapmaker/flatmap/__init__.py | 38 ++++++++++-------- mapmaker/flatmap/layers.py | 45 ++++++++++++---------- mapmaker/properties/pathways.py | 17 ++++---- mapmaker/sources/mbfbioscience/__init__.py | 13 ++++--- mapmaker/sources/powerpoint/__init__.py | 7 ++-- mapmaker/sources/svg/__init__.py | 4 +- 6 files changed, 71 insertions(+), 53 deletions(-) diff --git a/mapmaker/flatmap/__init__.py b/mapmaker/flatmap/__init__.py index fadafbd4..0f3e2fb8 100644 --- a/mapmaker/flatmap/__init__.py +++ b/mapmaker/flatmap/__init__.py @@ -288,16 +288,18 @@ def get_feature_by_geojson_id(self, geojson_id: int) -> Optional[Feature]: #========================================================================= return self.__features_by_geojson_id.get(geojson_id) - def new_feature(self, layer_id: str, geometry, properties, is_group=False, update_feature_with_id=False) -> Feature: - #=================================================================================================================== - self.__last_geojson_id += 1 + def new_feature(self, layer_id: str, geometry, properties, is_group=False) -> Optional[Feature]: + #=============================================================================================== properties['layer'] = layer_id self.properties_store.update_properties(properties) # Update from JSON properties file + if properties.id in self.__features_with_id: + log.error('Duplicate feature id', id=properties.id) + return None + self.__last_geojson_id += 1 feature = Feature(self.__last_geojson_id, geometry, properties, is_group=is_group) self.__features_by_geojson_id[feature.geojson_id] = feature if feature.id: - if update_feature_with_id or feature.id not in self.__features_with_id: - self.__features_with_id[feature.id] = feature + self.__features_with_id[feature.id] = feature if self.map_kind == MAP_KIND.FUNCTIONAL: if (name := properties.get('name', '')) != '': self.__features_with_name[f'{layer_id}/{name.replace(" ", "_")}'] = feature @@ -333,14 +335,14 @@ def __add_proxy_feature(self, feature: Feature, feature_model: str, proxy_seq: i if 'Polygon' not in feature.geometry.geom_type: log.warning('Proxy feature must have a polygon shape', type='proxy', models=feature_model, feature=feature) elif self.__bottom_exported_layer is not None: - self.__bottom_exported_layer.add_feature( - self.new_feature(self.__bottom_exported_layer.id, proxy_dot(feature.geometry, proxy_seq), { # type: ignore - 'id': f'proxy_{proxy_seq}_on_{feature.id}', - 'tile-layer': FEATURES_TILE_LAYER, - 'models': feature_model, - 'kind': 'proxy' - }) - ) + proxy_feature = self.new_feature(self.__bottom_exported_layer.id, proxy_dot(feature.geometry, proxy_seq), { # type: ignore + 'id': f'proxy_{proxy_seq}_on_{feature.id}', + 'tile-layer': FEATURES_TILE_LAYER, + 'models': feature_model, + 'kind': 'proxy' + }) + if proxy_feature is not None: + self.__bottom_exported_layer.add_feature(proxy_feature) def add_layer(self, layer: MapLayer): #==================================== @@ -410,6 +412,8 @@ def add_zoom_point(self, feature: Feature, description: Optional[str]=None) -> O 'kind': 'zoom-point', 'tile-layer': feature.properties['tile-layer'] }) + if zoom_point is None: + return if description is not None: zoom_point.set_property('label', description) if (models := feature.models) is not None: @@ -485,9 +489,11 @@ def __add_details(self): self.add_layer(layer) ## Put all this into 'features.py' as a function?? - def __new_detail_feature(self, layer_id, detail_layer, minzoom, geometry, properties): - #===================================================================================== + def __new_detail_feature(self, layer_id, detail_layer, minzoom, geometry, properties) -> Optional[Feature]: + #========================================================================================================== new_feature = self.new_feature(layer_id, geometry, properties) + if new_feature is None: + return new_feature.set_property('minzoom', minzoom) if properties.get('type') == 'nerve': new_feature.set_property('type', 'nerve-section') @@ -539,7 +545,7 @@ def __add_detail_features(self, layer, detail_layer, lowres_features): new_feature = self.__new_detail_feature(layer.id, detail_layer, minzoom, transform.transform_geometry(hires_feature.geometry), hires_feature.properties) - if new_feature.has_property('details'): + if new_feature is not None and new_feature.has_property('details'): extra_details.append(new_feature) # If hires features that we've just added also have details then add them diff --git a/mapmaker/flatmap/layers.py b/mapmaker/flatmap/layers.py index a4f5fff0..f5c83622 100644 --- a/mapmaker/flatmap/layers.py +++ b/mapmaker/flatmap/layers.py @@ -294,7 +294,7 @@ def add_group_features(self, group_name: str, features: list[Feature], 'tile-layer': tile_layer } - layer_features = [] # Features that will be added to the layer + layer_features: list[Feature] = [] # Features that will be added to the layer grouped_properties = { 'group': True, 'interior': True, @@ -305,8 +305,8 @@ def add_group_features(self, group_name: str, features: list[Feature], boundary_class = None boundary_lines = [] boundary_polygon = None - dividers = [] - regions = [] + dividers: list[shapely.LineString] = [] + regions: list[Feature] = [] debug_group = False child_class = None @@ -340,12 +340,14 @@ def add_group_features(self, group_name: str, features: list[Feature], child_class = feature.pop_property('children') grouped_properties.update(feature.properties) elif feature.get_property('region'): - regions.append(self.flatmap.new_feature(self.id, feature.geometry.representative_point(), feature.properties)) + region = self.flatmap.new_feature(self.id, feature.geometry.representative_point(), feature.properties) + if region is not None: + regions.append(region) elif feature.get_property('divider'): if feature.geom_type == 'LineString': - dividers.append(feature.geometry) + dividers.append(feature.geometry) # pyright: ignore[reportArgumentType] elif feature.geom_type == 'Polygon': - dividers.append(feature.geometry.boundary) + dividers.append(feature.geometry.boundary) # pyright: ignore[reportArgumentType] if feature.visible(): layer_features.append(feature) elif not feature.get_property('interior'): @@ -367,11 +369,9 @@ def add_group_features(self, group_name: str, features: list[Feature], raise GroupValueError('{}: {}'.format(group_name, str(err)), features) from None if boundary_polygon is not None: - layer_features.append( - self.flatmap.new_feature( - self.id, - boundary_polygon, - base_properties)) + feature = self.flatmap.new_feature(self.id, boundary_polygon, base_properties) + if feature is not None: + layer_features.append(feature) if len(dividers): # For all line dividers, if the end of a line is 'close to' another line @@ -382,7 +382,7 @@ def add_group_features(self, group_name: str, features: list[Feature], if debug_group: save_geometry(shapely.MultiLineString(dividers), 'dividers.wkt') - dividers.append(boundary_polygon.boundary) + dividers.append(boundary_polygon.boundary) # pyright: ignore[reportArgumentType] divider_lines = connect_dividers(dividers, debug_group) if debug_group: @@ -398,11 +398,14 @@ def add_group_features(self, group_name: str, features: list[Feature], if debug_group: save_geometry(polygon, f'polygon_{n}.wkt') prepared_polygon = shapely.prepared.prep(polygon) - region_id = None region_properties = base_properties.copy() + # So that any region doesn't have a duplicate id + region_properties.pop('id', None) for region in filter(lambda p: prepared_polygon.contains(p.geometry), regions): region_properties.update(region.properties) - layer_features.append(self.flatmap.new_feature(self.id, polygon, region_properties, update_feature_with_id=True)) + feature = self.flatmap.new_feature(self.id, polygon, region_properties) + if feature is not None: + layer_features.append(feature) break else: for feature in features: @@ -426,6 +429,8 @@ def add_group_features(self, group_name: str, features: list[Feature], # Construct a MultiPolygon containing all of the group's polygons # But only if the group contains a `.group` element... + # So that any grouped features don't have duplicate ids + grouped_properties.pop('id', None) feature_group = None # Our returned Feature if generate_group: grouped_polygon_features = [ feature for feature in features if feature.is_group ] @@ -442,10 +447,9 @@ def add_group_features(self, group_name: str, features: list[Feature], feature_group = self.flatmap.new_feature( self.id, shapely.MultiPolygon(grouped_polygons).buffer(0), - grouped_properties, is_group=True, update_feature_with_id=True) - layer_features.append(feature_group) - # So that any grouped lines don't have a duplicate id - grouped_properties.pop('id', None) + grouped_properties, is_group=True) + if feature_group is not None: + layer_features.append(feature_group) grouped_lines = [] for feature in grouped_polygon_features: @@ -459,8 +463,9 @@ def add_group_features(self, group_name: str, features: list[Feature], feature_group = self.flatmap.new_feature( self.id, shapely.MultiLineString(grouped_lines), - grouped_properties, is_group=True, update_feature_with_id=True) - layer_features.append(feature_group) + grouped_properties, is_group=True) + if feature_group is not None: + layer_features.append(feature_group) # Feature specific properties have precedence over group's default_properties = base_properties.copy() diff --git a/mapmaker/properties/pathways.py b/mapmaker/properties/pathways.py index a211bc01..66879481 100644 --- a/mapmaker/properties/pathways.py +++ b/mapmaker/properties/pathways.py @@ -842,10 +842,11 @@ def __route_network_connectivity(self, network: Network): if 'id' not in properties and path_model is not None: properties['id'] = path_model.replace(':', '_').replace('/', '_') feature = self.__flatmap.new_feature('pathways', geometric_shape.geometry, properties) - path_geojson_ids.append(feature.geojson_id) - layer.add_feature(feature) - if path_taxons is None: - path_taxons = feature.get_property('taxons') + if feature is not None: + path_geojson_ids.append(feature.geojson_id) + layer.add_feature(feature) + if path_taxons is None: + path_taxons = feature.get_property('taxons') for geometric_shape in geometric_shapes: properties = DEFAULT_PATH_PROPERTIES.copy() | added_properties @@ -858,8 +859,9 @@ def __route_network_connectivity(self, network: Network): if path_taxons is not None: properties['taxons'] = path_taxons feature = self.__flatmap.new_feature('pathways', geometric_shape.geometry, properties) - path_geojson_ids.append(feature.geojson_id) - layer.add_feature(feature) + if feature is not None: + path_geojson_ids.append(feature.geojson_id) + layer.add_feature(feature) nerve_feature_ids = routed_path.nerve_feature_ids nerve_features = [self.__flatmap.get_feature(nerve_id) for nerve_id in nerve_feature_ids] @@ -896,7 +898,8 @@ def __route_network_connectivity(self, network: Network): nerve_polygon_feature = self.__flatmap.new_feature( 'pathways', shapely.geometry.Polygon(feature.geometry.coords).buffer(0), properties) - layer.features.append(nerve_polygon_feature) + if nerve_polygon_feature is not None: + layer.features.append(nerve_polygon_feature) def generate_connectivity(self, networks: Iterable[Network]): #============================================================ diff --git a/mapmaker/sources/mbfbioscience/__init__.py b/mapmaker/sources/mbfbioscience/__init__.py index 9453e01d..a1313d82 100644 --- a/mapmaker/sources/mbfbioscience/__init__.py +++ b/mapmaker/sources/mbfbioscience/__init__.py @@ -146,12 +146,13 @@ def process(self): if anatomical_id is not None: properties['models'] = anatomical_id feature = self.flatmap.new_feature(self.id, geometry, properties) - feature.set_property('dataset', self.__sparc_dataset) - feature.set_property('source', self.href) - self.__layer.add_feature(feature) - if anatomical_id == self.__boundary_id: - boundary_geometry = feature.geometry - self.__layer.boundary_feature = feature + if feature is not None: + feature.set_property('dataset', self.__sparc_dataset) + feature.set_property('source', self.href) + self.__layer.add_feature(feature) + if anatomical_id == self.__boundary_id: + boundary_geometry = feature.geometry + self.__layer.boundary_feature = feature if boundary_geometry is not None and boundary_geometry.geom_type == 'Polygon': # Save boundary in case transformed image is used for details self.__boundary_geometry = boundary_geometry diff --git a/mapmaker/sources/powerpoint/__init__.py b/mapmaker/sources/powerpoint/__init__.py index bc4ad48e..f23b7c01 100644 --- a/mapmaker/sources/powerpoint/__init__.py +++ b/mapmaker/sources/powerpoint/__init__.py @@ -128,9 +128,10 @@ def __process_shape_list(self, shapes: TreeList[Shape]) -> list[Feature]: pass elif not properties.get('exclude', False): feature = self.flatmap.new_feature(self.id, shape.geometry, properties) - features.append(feature) - shape.set_property('geojson_id', feature.geojson_id) - shape.set_property('feature', feature) + if feature is not None: + features.append(feature) + shape.set_property('geojson_id', feature.geojson_id) + shape.set_property('feature', feature) return features #=============================================================================== diff --git a/mapmaker/sources/svg/__init__.py b/mapmaker/sources/svg/__init__.py index a390b050..0a807418 100644 --- a/mapmaker/sources/svg/__init__.py +++ b/mapmaker/sources/svg/__init__.py @@ -265,7 +265,9 @@ def __process_shape_list(self, shapes: TreeList[Shape], depth) -> list[Feature]: if isinstance(shape, TreeList): self.__process_shape_list(shape, depth+1) elif not shape.properties.get('exclude', False): - features.append(self.flatmap.new_feature(self.id, shape.geometry, shape.properties)) + feature = self.flatmap.new_feature(self.id, shape.geometry, shape.properties) + if feature is not None: + features.append(feature) self.add_group_features(f'SVG_{depth}', features, outermost=(depth==0)) return features From 59c40192a8722a39a83b6b676d0b317db790197c Mon Sep 17 00:00:00 2001 From: David Brooks Date: Thu, 28 Aug 2025 13:59:32 +1200 Subject: [PATCH 5/6] Fix code -- we have a `dict` not a `Shape` object. --- mapmaker/flatmap/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mapmaker/flatmap/__init__.py b/mapmaker/flatmap/__init__.py index 0f3e2fb8..f041e186 100644 --- a/mapmaker/flatmap/__init__.py +++ b/mapmaker/flatmap/__init__.py @@ -292,8 +292,8 @@ def new_feature(self, layer_id: str, geometry, properties, is_group=False) -> Op #=============================================================================================== properties['layer'] = layer_id self.properties_store.update_properties(properties) # Update from JSON properties file - if properties.id in self.__features_with_id: - log.error('Duplicate feature id', id=properties.id) + if (id:=properties.get('id')) is not None and id in self.__features_with_id: + log.error('Duplicate feature id', id=id) return None self.__last_geojson_id += 1 feature = Feature(self.__last_geojson_id, geometry, properties, is_group=is_group) From 43f64d6c8e6b24d04f7c88583892a7e91af1a522 Mon Sep 17 00:00:00 2001 From: David Brooks Date: Thu, 28 Aug 2025 14:44:24 +1200 Subject: [PATCH 6/6] Don't try to set an `id` on path and region features (#160). --- mapmaker/flatmap/layers.py | 5 ++++- mapmaker/properties/pathways.py | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/mapmaker/flatmap/layers.py b/mapmaker/flatmap/layers.py index f5c83622..fd1ef626 100644 --- a/mapmaker/flatmap/layers.py +++ b/mapmaker/flatmap/layers.py @@ -340,7 +340,10 @@ def add_group_features(self, group_name: str, features: list[Feature], child_class = feature.pop_property('children') grouped_properties.update(feature.properties) elif feature.get_property('region'): - region = self.flatmap.new_feature(self.id, feature.geometry.representative_point(), feature.properties) + region_properties = feature.properties.copy() + # So that any region doesn't have a duplicate id + region_properties.pop('id', None) + region = self.flatmap.new_feature(self.id, feature.geometry.representative_point(), region_properties) if region is not None: regions.append(region) elif feature.get_property('divider'): diff --git a/mapmaker/properties/pathways.py b/mapmaker/properties/pathways.py index 66879481..e833781b 100644 --- a/mapmaker/properties/pathways.py +++ b/mapmaker/properties/pathways.py @@ -839,8 +839,6 @@ def __route_network_connectivity(self, network: Network): properties['label'] = '\n'.join(labels) elif path_model is not None: properties['label'] = path.label - if 'id' not in properties and path_model is not None: - properties['id'] = path_model.replace(':', '_').replace('/', '_') feature = self.__flatmap.new_feature('pathways', geometric_shape.geometry, properties) if feature is not None: path_geojson_ids.append(feature.geojson_id)