Keep shapely for polygon containment only and drop matplotlib - #42
Keep shapely for polygon containment only and drop matplotlib#42amorfinv wants to merge 3 commits into
Conversation
Every Shape now carries a shapely geometry in (lat, lon) space and containment is one vectorized contains_xy call in the base class. Circles become geographic N-gons via kwikpos, with the vertex count sized to keep the border within 0.05 NM of the true circle.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
minisky | 46544fc | Aug 09 2026, 06:27 AM |
| altitude bounds [m]. | ||
| """ | ||
|
|
||
| # Maximum distance [nm] between the polygon border and the true circle |
There was a problem hiding this comment.
I am bit unsure about approaching circles this way.
One option is to just buffer a point (https://shapely.readthedocs.io/en/stable/reference/shapely.buffer.html) into a LinearRing. But I think for large radius, it will look like an eclipse
Another option could be to just drop the circle shape as it is similar to polygon. Users can already approximate a circle with a poly command.
There was a problem hiding this comment.
Hmm I'm not sure we need to use shapely here, the old implementation of kwikdist looks alright to me.
Maybe switch to using qdrdist or latlondist for large radius or near the poles?
There was a problem hiding this comment.
Also I see that you now require all base classes of Shape to provide a shapely geometry. I think if we don't intend to use the shapely geometry elsewhere in the code we can just define a protected _geom (for Polygon) and not define it at all for other shapes
There was a problem hiding this comment.
I was thinking for an urban airspace plugin with many geofences it will be nice if shapes are already in shapely to be able to use rtree for quick spatial operations. perhaps plugins can make their own shapes though.
There was a problem hiding this comment.
Yes but the reason why I didn't want to force every shape to own a shapely geometry is more to do with the fact that shapely itself only works with planar Cartesian geometry. With the current latitude/longitude representation, we need to be very careful about two cases:
- polygon with vertices
(10, 170),(10, -170),(-10, -170), and(-10, 170)that encloses the antimeridian. if we switch to usingshapely.box,(0, 0)is considered inside the polygon, but should be outside. - a 100 NM circle centred at the north pole. right now the new implementation generates vertices above 90° latitude and produces an invalid shapely polygon.
I think we should avoid making the planar shapely geometry as a core invariant, and avoid exposing it in general to prevent misuse. For:
- box and circle: I actually prefer the old implementation, which should solve the two issues
- polygon: I think we can still use shapely's
contains_xy, but keep it protected. for the antimeridian case we should follow RFC 7946 §3.1.9 to split the polygon into two chunks internally. for the polygons that enclose the poles, I can't think of a good solution so I'm geared towards rejecting it outright
Re shapely's STRTree: I haven't used it so I can't comment on whether it will bring any performance improvement (I suspect since we are dealing with a relatively small number of aircraft and geofences, it may not be necessary)
There was a problem hiding this comment.
I have ran simulation with a large amount of aircraft and geofences where it is beneficial to have the Rtree. However, perhaps that can be left for a plugin.
I will work on changing the scope of this PR sometime next week
| super().__init__(name, coordinates) | ||
| self.geom = shapely.LineString(np.reshape(coordinates, (-1, 2))) | ||
|
|
||
| def checkInside(self, lat: np.ndarray, lon: np.ndarray, alt: np.ndarray) -> np.ndarray: |
There was a problem hiding this comment.
Small nit, but it is category error to check whether an aircraft is inside a line. To prevent this we should remove the Shape base class altogether to avoid implementation inheritance in general. We could use explicit structural subtyping with Protocols, something like:
class HasArea(Protocol):
def contains(self, lat, lon, alt):
...
class Box(HasArea): ...
class Circle(HasArea): ...
class Polygon(HasArea): ...
# but not for line.|
I have managed to rescope this PR to keep most of the original functionality and only use shapely for |
| return self.define_area(name, "LINE", coords) | ||
|
|
||
| def checkInside( | ||
| def contains( |
There was a problem hiding this comment.
I haven't taken a look at what parts of the code uses AreaFilter but it looks like we are still associating a "line" (which has no area) as part of an "area filter". I would get rid of Line from AreaFilter.shapes and let callers handle it. Maybe that is too big of a refactor so I would just merge it
There was a problem hiding this comment.
I'll give it a check later today but these shapes are mostly helpers for the user. I don't think they are that important for calculation. I can rename this to shapes or something
I recall that in bluesky there is a plugin which uses areafilter to create an experiment area. So basically anything outside tje shape is not logged and quickly deleted.
Closes #32
matplotlibwas a dependency with just a single geometric predicate: the point-in-polygon test inPoly.checkInside. This PR unifies all area shapes onshapelygeometries instead, so containment is one vectorizedshapely.contains_xycall in theShapebase class, andmatplotlibis dropped from the dependencies (it remains in the environment only as a transitive dependency of openap).Each shape now builds ashapelygeometry in (lat, lon) space at construction:Boxviashapely.boxon its sorted corners,Polydirectly from its vertex list, andCircleas a geographic N-gon whose vertices are placed withkwikpos, keeping the cos(lat) longitude scaling correct. The circle's vertex count adapts to its radius so the polygon border stays within 0.05 NM of the true circle at any size, clamped to 36–720 vertices.Linekeeps an explicit all-FalsecheckInsidesince it has zero area.This PR swaps
matplotlibforshapelyas the point-in-polygon backend:Polynow holds a privateshapely.Polygonand usescontains_xyinstead of amatplotlib.path.Path, which letsmatplotlibcome out of the dependencies.BoxandCirclekeep their existing geographic math unchanged (bounds comparison and kwikdist), sinceshapely's flat-plane assumption breaks near the poles and at the antimeridian. Polygons that hit those cases, crossing the 180° line or winding around a pole, are now rejected at definition time with an error rather than silently returning wrong answers.The
Shapebase class is removed in favor of aHasAreaprotocol implemented only byBox,Circle, andPoly, soLineno longer carries acheckInsidemethod for a question it can't meaningfully answer; this drops the public Shape,.raw, and.kind(), which nothing in the repo used.