Replace feature in __features_with_id when a new feature is created due to .group element containment

[napakalas]

#160

This fix is related to the previous issue regarding the .group element. Features in FlatMap.__features_with_id are updated when a new feature with the same id has both group and interior properties set to True.

[dbrnz]

Hmm, you presumably haven't seen this comment?

[dbrnz]

Are duplicate ids to do with grouping of features, either via group or interior markup?

[napakalas]

Hmm, you presumably haven't seen this #160 (comment)?

Right, I hadn’t seen that comment. Thanks for highlighting it.

Are duplicate ids to do with grouping of features, either via group or interior markup?

Yes, the newly created feature is based on an already created feature via the group.
Alright then, I’ll update the PR tomorrow and fix the flatmap source based on the logs.

[dbrnz]

Yes, the newly created feature is based on an already created feature via the group.

If the group is given an explicit id via markup it should be different from ids of its members; otherwise the group feature should not inherit an id from a member. Conversely, members of a group should not inherit the group's id -- this should have been taken care of via NON_INHERITED_PROPERTIES in sources/svg/__init__.py.

[napakalas]

There are three cases related to duplicate IDs:

• Rendering path geometric shapes consisting of multiple lines. Example code block:
  

  flatmap-maker/mapmaker/properties/pathways.py

  Lines 824 to 862 in 3f0807c

  	for geometric_shape in geometric_shapes:
  	if geometric_shape.properties.get('type') not in ['arrow', 'junction']:
  	properties = DEFAULT_PATH_PROPERTIES.copy() | added_properties
  	if routed_path.centrelines is not None:
  	# The list of nerve models that the path is associated with
  	properties['nerves'] = routed_path.centrelines_model
  	properties.update(self.__line_properties(path_id))
  	path_model = path.models
  	if settings.get('authoring', False):
  	labels = []
  	if path_model is not None:
  	labels.append(f'Models: {path_model}')
  	labels.append(f'Label: {path.label}')
  	labels.append(f'Number: {route_number}')
  	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)
  	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
  	properties.update(geometric_shape.properties)
  	if properties.get('type') in ['arrow', 'junction']:
  	properties['kind'] = path.path_type.viewer_kind
  	if routed_path.centrelines is not None:
  	# The list of nerve models that the path is associated with
  	properties['nerves'] = routed_path.centrelines_model
  	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)

• Group-related elements. Example element in the SVG file:

  <g transform="translate(0,20)">
      <title>.id(C1)</title>
      <polygon ...>
        <title>.group class(spinal_1) id(spinal_1-1)</title>
      </polygon>
      ...
  </g>

Related code:

flatmap-maker/mapmaker/flatmap/layers.py

Lines 429 to 463 in 3f0807c

	feature_group = None # Our returned Feature
	if generate_group:
	grouped_polygon_features = [ feature for feature in features if feature.is_group ]
	for feature in layer_features:
	grouped_polygon_features.append(feature)
	grouped_polygons = []
	for feature in grouped_polygon_features:
	if feature.geom_type == 'Polygon':
	grouped_polygons.append(feature.geometry)
	elif feature.geom_type == 'MultiPolygon':
	grouped_polygons.extend(list(feature.geometry.geoms)) # type: ignore
	if len(grouped_polygons):
	feature_group = self.flatmap.new_feature(
	self.id,
	shapely.MultiPolygon(grouped_polygons).buffer(0),
	grouped_properties, is_group=True)
	layer_features.append(feature_group)
	# So that any grouped lines don't have a duplicate id
	grouped_properties.pop('id', None)
	grouped_lines = []
	for feature in grouped_polygon_features:
	if feature.get_property('tile-layer') != PATHWAYS_TILE_LAYER:
	if feature.geom_type == 'LineString':
	grouped_lines.append(feature.geometry)
	elif feature.geom_type == 'MultiLineString':
	grouped_lines.extend(list(feature.geometry.geoms)) # type: ignore
	if len(grouped_lines): ## should polygons take precedence over lines???
	## at least for assigning ID...
	feature_group = self.flatmap.new_feature(
	self.id,
	shapely.MultiLineString(grouped_lines),
	grouped_properties, is_group=True)
	layer_features.append(feature_group)

• Region-related elements. Example in the SVG:

<polygon ...>
    <title>.region id(digestive_21-1)</title>
</polygon>

Related code:

flatmap-maker/mapmaker/flatmap/layers.py

Lines 342 to 343 in 3f0807c

	elif feature.get_property('region'):
	regions.append(self.flatmap.new_feature(self.id, feature.geometry.representative_point(), feature.properties))

Therefore, updating Flatmap.new_feature() to return None when a duplicate ID is found will break the rendering of paths.

The duplication mechanism in group and region elements occurs because a feature is first created and stored as a pair (id, feature) in Flatmap.__features_with_id when calling Flatmap.new_feature(). Later, when Maplayer.add_group_features() is called (in the lines referenced above), new features are created based on these initial group and region features. The new features receive new geojsonids and are stored in layers.

Because nodes in ./pathways are derived from Flatmap.__features_with_id while features in ./annotations are came from layers, this leads to inconsistencies.

I am still not completely sure what the correct form of group and region annotations in the SVG file should be. For Flatmap.__features_with_id, would it be more appropriate to update it with the new features that are ultimately stored in layers and log the duplicate?

[dbrnz]

Thanks for the research! I'll think about the best way to resolve this and get back (it may not be until the afternoon as I am travelling earlier in the day).

[napakalas]

The proposed solution in this commit introduces a new argument,
update_feature_with_id, to FlatMap.new_feature(). When set to True,
the new feature will also be added or updated in __features_with_id.