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.

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

Fix inner boundaries for polygon #355 #356

Merged
merged 3 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
108 changes: 53 additions & 55 deletions fastkml/geometry.py
Original file line number Diff line number Diff line change
Expand Up @@ -911,15 +911,15 @@ class InnerBoundaryIs(_XMLObject):
"""Represents the inner boundary of a polygon in KML."""

_default_nsid = config.KML
kml_geometries: List[LinearRing]
kml_geometry: Optional[LinearRing]

def __init__(
self,
*,
ns: Optional[str] = None,
name_spaces: Optional[Dict[str, str]] = None,
geometries: Optional[Iterable[geo.LinearRing]] = None,
kml_geometries: Optional[Iterable[LinearRing]] = None,
geometry: Optional[geo.LinearRing] = None,
kml_geometry: Optional[LinearRing] = None,
**kwargs: Any,
) -> None:
"""
Expand All @@ -931,37 +931,31 @@ def __init__(
The namespace for the KML element, by default None.
name_spaces : Optional[Dict[str, str]], optional
The namespace dictionary for the KML element, by default None.
geometries : Optional[Iterable[geo.LinearRing]], optional
The geometries to be converted to KML geometries, by default None.
kml_geometries : Optional[Iterable[LinearRing]], optional
The KML geometries, by default None.
geometry : Optional[geo.LinearRing], optional
The geometry to be converted to a KML geometry, by default None.
kml_geometry : Optional[LinearRing], optional
The KML geometry, by default None.
**kwargs : Any
Additional keyword arguments.

Raises
------
GeometryError
If both `geometries` and `kml_geometries` are provided.
If both `geometry` and `kml_geometry` are provided.

Notes
-----
- If `geometries` is provided, it will be converted to KML geometries and
stored in `kml_geometries`.
- If `geometries` is not provided, `kml_geometries` will be an empty list.
- If `geometry` is provided, it will be converted to KML geometries and
stored in `kml_geometry`.
- If `geometry` and `kml_geometry` are both provided, a GeometryError will be
raised.

"""
if geometries is not None and kml_geometries is not None:
if geometry is not None and kml_geometry is not None:
raise GeometryError(MsgMutualExclusive)
if kml_geometries is None:
kml_geometries = (
[
LinearRing(ns=ns, name_spaces=name_spaces, geometry=lr)
for lr in geometries
]
if geometries
else None
)
self.kml_geometries = list(kml_geometries) if kml_geometries else []
if kml_geometry is None:
kml_geometry = LinearRing(ns=ns, name_spaces=name_spaces, geometry=geometry)
self.kml_geometry = kml_geometry
super().__init__(
ns=ns,
name_spaces=name_spaces,
Expand All @@ -970,15 +964,15 @@ def __init__(

def __bool__(self) -> bool:
"""Return True if any of the inner boundary geometries exist."""
return any(b.geometry for b in self.kml_geometries)
return bool(self.kml_geometry)

def __repr__(self) -> str:
"""Create a string (c)representation for InnerBoundaryIs."""
return (
f"{self.__class__.__module__}.{self.__class__.__name__}("
f"ns={self.ns!r}, "
f"name_spaces={self.name_spaces!r}, "
f"kml_geometries={self.kml_geometries!r}, "
f"kml_geometry={self.kml_geometry!r}, "
f"**{self._get_splat()},"
")"
)
Expand All @@ -989,26 +983,24 @@ def get_tag_name(cls) -> str:
return "innerBoundaryIs"

@property
def geometries(self) -> Optional[Iterable[geo.LinearRing]]:
def geometry(self) -> Optional[geo.LinearRing]:
"""
Return the list of LinearRing objects representing the inner boundary.

If no inner boundary geometries exist, returns None.
"""
if not self.kml_geometries:
return None
return [lr.geometry for lr in self.kml_geometries if lr.geometry]
return self.kml_geometry.geometry if self.kml_geometry else None


registry.register(
InnerBoundaryIs,
item=RegistryItem(
ns_ids=("kml",),
classes=(LinearRing,),
attr_name="kml_geometries",
attr_name="kml_geometry",
node_name="LinearRing",
get_kwarg=xml_subelement_list_kwarg,
set_element=xml_subelement_list,
get_kwarg=xml_subelement_kwarg,
set_element=xml_subelement,
),
)

Expand Down Expand Up @@ -1042,8 +1034,8 @@ class Polygon(_Geometry):
https://developers.google.com/kml/documentation/kmlreference#polygon
"""

outer_boundary_is: Optional[OuterBoundaryIs]
inner_boundary_is: Optional[InnerBoundaryIs]
outer_boundary: Optional[OuterBoundaryIs]
inner_boundaries: Optional[List[InnerBoundaryIs]]

def __init__(
self,
Expand All @@ -1055,8 +1047,8 @@ def __init__(
extrude: Optional[bool] = None,
tessellate: Optional[bool] = None,
altitude_mode: Optional[AltitudeMode] = None,
outer_boundary_is: Optional[OuterBoundaryIs] = None,
inner_boundary_is: Optional[InnerBoundaryIs] = None,
outer_boundary: Optional[OuterBoundaryIs] = None,
inner_boundaries: Optional[Iterable[InnerBoundaryIs]] = None,
geometry: Optional[geo.Polygon] = None,
**kwargs: Any,
) -> None:
Expand All @@ -1079,9 +1071,9 @@ def __init__(
The tessellate flag of the element.
altitude_mode : Optional[AltitudeMode]
The altitude mode of the element.
outer_boundary_is : Optional[OuterBoundaryIs]
outer_boundary : Optional[OuterBoundaryIs]
The outer boundary of the element.
inner_boundary_is : Optional[InnerBoundaryIs]
inner_boundaries : Optional[Iterable[InnerBoundaryIs]]
The inner boundaries of the element.
geometry : Optional[geo.Polygon]
The geometry object of the element.
Expand All @@ -1098,13 +1090,15 @@ def __init__(
None

"""
if outer_boundary_is is not None and geometry is not None:
if outer_boundary is not None and geometry is not None:
raise GeometryError(MsgMutualExclusive)
if geometry is not None:
outer_boundary_is = OuterBoundaryIs(geometry=geometry.exterior)
inner_boundary_is = InnerBoundaryIs(geometries=geometry.interiors)
self.outer_boundary_is = outer_boundary_is
self.inner_boundary_is = inner_boundary_is
outer_boundary = OuterBoundaryIs(geometry=geometry.exterior)
inner_boundaries = [
InnerBoundaryIs(geometry=interior) for interior in geometry.interiors
]
self.outer_boundary = outer_boundary
self.inner_boundaries = list(inner_boundaries) if inner_boundaries else []
super().__init__(
ns=ns,
name_spaces=name_spaces,
Expand All @@ -1126,7 +1120,7 @@ def __bool__(self) -> bool:
True if the outer boundary is defined, False otherwise.

"""
return bool(self.outer_boundary_is)
return bool(self.outer_boundary)

@property
def geometry(self) -> Optional[geo.Polygon]:
Expand All @@ -1139,15 +1133,19 @@ def geometry(self) -> Optional[geo.Polygon]:
The geometry object representing the geometry of the Polygon.

"""
if not self.outer_boundary_is:
if not self.outer_boundary:
return None
if not self.inner_boundary_is:
if not self.inner_boundaries:
return geo.Polygon.from_linear_rings(
cast(geo.LinearRing, self.outer_boundary_is.geometry),
cast(geo.LinearRing, self.outer_boundary.geometry),
)
return geo.Polygon.from_linear_rings( # type: ignore[misc]
cast(geo.LinearRing, self.outer_boundary_is.geometry),
*self.inner_boundary_is.geometries,
return geo.Polygon.from_linear_rings(
cast(geo.LinearRing, self.outer_boundary.geometry),
*[
interior.geometry
for interior in self.inner_boundaries
if interior.geometry is not None
],
)

def __repr__(self) -> str:
Expand All @@ -1169,8 +1167,8 @@ def __repr__(self) -> str:
f"extrude={self.extrude!r}, "
f"tessellate={self.tessellate!r}, "
f"altitude_mode={self.altitude_mode}, "
f"outer_boundary_is={self.outer_boundary_is!r}, "
f"inner_boundary_is={self.inner_boundary_is!r}, "
f"outer_boundary={self.outer_boundary!r}, "
f"inner_boundaries={self.inner_boundaries!r}, "
f"**{self._get_splat()!r},"
")"
)
Expand All @@ -1181,7 +1179,7 @@ def __repr__(self) -> str:
item=RegistryItem(
ns_ids=("kml",),
classes=(OuterBoundaryIs,),
attr_name="outer_boundary_is",
attr_name="outer_boundary",
node_name="outerBoundaryIs",
get_kwarg=xml_subelement_kwarg,
set_element=xml_subelement,
Expand All @@ -1192,10 +1190,10 @@ def __repr__(self) -> str:
item=RegistryItem(
ns_ids=("kml",),
classes=(InnerBoundaryIs,),
attr_name="inner_boundary_is",
attr_name="inner_boundaries",
node_name="innerBoundaryIs",
get_kwarg=xml_subelement_kwarg,
set_element=xml_subelement,
get_kwarg=xml_subelement_list_kwarg,
set_element=xml_subelement_list,
),
)

Expand Down
36 changes: 20 additions & 16 deletions tests/geometries/boundaries_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,32 @@ def test_inner_boundary(self) -> None:
"""Test the init method."""
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
inner_boundary = InnerBoundaryIs(
kml_geometries=[LinearRing(kml_coordinates=Coordinates(coords=coords))],
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Test case for handling multiple LinearRings in innerBoundaryIs

This test now correctly checks that only the first LinearRing is used when multiple are present in the XML. Consider adding a warning or log message in the implementation to inform users when additional LinearRings are ignored.


assert inner_boundary.geometries == [geo.LinearRing(coords)]
def test_inner_boundary(self) -> None:
"""Test the init method and __bool__."""
coords = ((1, 2), (2, 0), (0, 0), (1, 2))
inner_boundary = InnerBoundaryIs(
kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
)

assert inner_boundary.geometry == geo.LinearRing(coords)
assert bool(inner_boundary) is True
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Duplicate method definition: test_inner_boundary

The method test_inner_boundary is defined twice in the TestBoundaries class (once at lines 63-65 and again at lines 66-74). This will cause the first definition to be overwritten and may lead to confusion or unintended behavior. Please remove the duplicate method or rename one of them to have a unique name.

Apply this diff to remove the duplicate method:

-def test_inner_boundary(self) -> None:
-    """Test the init method."""
-    coords = ((1, 2), (2, 0), (0, 0), (1, 2))
-    inner_boundary = InnerBoundaryIs(
-        kml_geometry=LinearRing(kml_coordinates=Coordinates(coords=coords)),
-    )
-
-    assert inner_boundary.geometry == geo.LinearRing(coords)
-    assert inner_boundary.to_string(prettyprint=False).strip() == (
-        '<kml:innerBoundaryIs xmlns:kml="http://www.opengis.net/kml/2.2">'
-        "<kml:LinearRing><kml:coordinates>"
-        "1.000000,2.000000 2.000000,0.000000 0.000000,0.000000 1.000000,2.000000"
-        "</kml:coordinates></kml:LinearRing></kml:innerBoundaryIs>"
-    )

Committable suggestion was skipped due to low confidence.

assert inner_boundary.to_string(prettyprint=False).strip() == (
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix indentation error at line 75

There is an indentation issue starting at line 75, which leads to a SyntaxError: Unexpected indentation. Please check the indentation of your code, especially around line 75, to ensure it adheres to Python's syntax rules.

🧰 Tools
🪛 Ruff

75-75: SyntaxError: Unexpected indentation

'<kml:innerBoundaryIs xmlns:kml="http://www.opengis.net/kml/2.2">'
"<kml:LinearRing><kml:coordinates>"
"1.000000,2.000000 2.000000,0.000000 0.000000,0.000000 1.000000,2.000000"
"</kml:coordinates></kml:LinearRing></kml:innerBoundaryIs>"
)

def test_read_inner_boundary(self) -> None:
"""Test the from_string method."""
def test_read_inner_boundary_multiple_linestrings(self) -> None:
"""
Test the from_string method.

When there are multiple LinearRings in the innerBoundaryIs element
only the first one is used.
"""
inner_boundary = InnerBoundaryIs.class_from_string(
'<kml:innerBoundaryIs xmlns:kml="http://www.opengis.net/kml/2.2">'
"<kml:LinearRing>"
Expand All @@ -85,18 +98,9 @@ def test_read_inner_boundary(self) -> None:
"</kml:innerBoundaryIs>",
)

assert inner_boundary.geometries == [
geo.LinearRing(((1, 4), (2, 0), (0, 0), (1, 4))),
geo.LinearRing(
(
(-122.366212, 37.818977, 30),
(-122.365424, 37.819294, 30),
(-122.365704, 37.819731, 30),
(-122.366488, 37.819402, 30),
(-122.366212, 37.818977, 30),
),
),
]
assert inner_boundary.geometry == geo.LinearRing(
((1, 4), (2, 0), (0, 0), (1, 4)),
)


class TestBoundariesLxml(Lxml, TestBoundaries):
Expand Down
6 changes: 4 additions & 2 deletions tests/geometries/geometry_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def test_extrude(self) -> None:

assert g.extrude is True

def test_tesselate(self) -> None:
def test_tessellate(self) -> None:
doc = """<kml:Point xmlns:kml="http://www.opengis.net/kml/2.2">
<kml:coordinates>0.000000,1.000000</kml:coordinates>
<kml:tessellate>1</kml:tessellate>
Expand Down Expand Up @@ -224,7 +224,8 @@ def test_multipolygon(self) -> None:

g = MultiGeometry.class_from_string(doc)

assert len(g.geometry) == 2 # type: ignore[arg-type]
assert g.geometry is not None
assert len(g.geometry) == 2

def test_geometrycollection(self) -> None:
doc = """
Expand Down Expand Up @@ -464,6 +465,7 @@ def test_create_kml_geometry_polygon(self) -> None:
g = create_kml_geometry(geo.Polygon([(0, 0), (1, 1), (1, 0), (0, 0)]))

assert isinstance(g, Polygon)
assert g.geometry is not None
assert g.geometry.__geo_interface__ == {
"type": "Polygon",
"bbox": (0.0, 0.0, 1.0, 1.0),
Expand Down
4 changes: 3 additions & 1 deletion tests/geometries/polygon_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,9 @@ def test_empty_polygon(self) -> None:
polygon = Polygon.class_from_string(doc)

assert not polygon.geometry
assert polygon.outer_boundary_is is not None
assert polygon.outer_boundary is not None
cleder marked this conversation as resolved.
Show resolved Hide resolved
assert isinstance(polygon.outer_boundary, LinearRing)
assert len(polygon.inner_boundaries) == 0
assert "tessellate>1</" in polygon.to_string()


Expand Down
Loading
Loading