fix: exposing simplify from Geometry.geojson by kwargs in explore

[MarkusZehner]

Fix for issue #240

Simple workaround to expose simplify to the explore function.

[Kirill888]

@MarkusZehner thanks for this. Can we make it more "finished" though. Ideally we should address #240 not just provide an undocumented work-around.

I think we need simplify: float | Literal["auto"] = "auto" argument exposed as an optional parameter, with it's own entry in the function signature and it's own documentation string.

The default "auto" case should resolve to simplify=0 if geometry has "few enough" points (let's say 100, that should display quickly enough). Otherwise it should be simplify=min(0.05, res*0.1). Can you implement that and check if this fixes your problem case?

Please ensure the following:

1. Run black on the file you have edited
2. Do not make a new commit, instead git amend your current commit with the new changes, then use git push --force-with-lease to submit your new work
3. Please make sure git commit message width fits within 80 char column and follows this pattern

   Summary of Changes #240 (<80 char)
   
   ^ empty line and more description when needed.
   Wrap text to make sure commit message fits within 80 char column limit.
   Reference issue you are fixing with #240 syntax, usually in the summary line
   

pinging @robbibt as the original author of .explore

[MarkusZehner]

I am running into some problems calling Geometry.points

---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)

File /python3.12/site-packages/odc/geo/geom.py:586, in Geometry.points(self)
    585 @property
--> [586](/python3.12/site-packages/odc/geo/geom.py:586) def points(self) -> CoordList: return self.coords

File /python3.12/site-packages/odc/geo/geom.py:584, in Geometry.coords(self)
    583 @property
--> [584](/python3.12/site-packages/odc/geo/geom.py:584) def coords(self) -> CoordList: return cast(list[tuple[float, float]], list(self.geom.coords))

File /python3.12/site-packages/shapely/geometry/polygon.py:266, in Polygon.coords(self)
    263 @property
    264 def coords(self):
    265     """Not implemented for polygons."""
--> [266](/python3.12/site-packages/shapely/geometry/polygon.py:266)     raise NotImplementedError(
    267         "Component rings have coordinate sequences, but the polygon does not"
    268     )

NotImplementedError: Component rings have coordinate sequences, but the polygon does not

However, len(geometry.exterior.coords) does give the number of coords. Should this be checked when calling Geometry.points or Geometry.coords?

[Kirill888]

@MarkusZehner good point, I forgot it's not that easy to find "number of points" making up an arbitrary geometry object. Just counting exterior points of a polygon is not good enough for a polygon with fractal holes within it. This approach will also break for more complex geometries like feature collections.

I'm not aware of an obvious shapely method that can return number of nodes geometry is made up of. Every type has a different method for exposing point nodes it's made of, and requires marshaling all those points through C-Python interface just to reduce that back to a single number. We can probably use .wkb method to get a rough idea of geometry "weight", but for now, let's just skip that and always perform simplification when in "auto" mode, but make it more robust by selecting appropriate simplification threshold based on resolution used.

[Kirill888]

Also, please rebase against the current develop branch to get rid of CI errors

[MarkusZehner]

@Kirill888 how about using shapely.count_coordinates, also the resolution is dependent on crs which hinders a simple threshold.

While setting up _auto_simplify, I stumbled upon _auto_resolution. By docs and comments, both aim for the same 100pt/side, while not resulting in the same value. For simplicity I would opt to use _auto_resolution in explore.

[Kirill888]

Too much detail, res is not an argument, we should not mention it, 100 is an implementation detail, also no need to mention. Mostly this will be used as simplify=0, so explain why would one use it.

[Kirill888]

this doesn't feel right:

• converting to lonlat twice
• converting even when not needed (i.e. s=0.0 case)
• function not generic enough to be at module level, or be a function at all

Maybe it makes more sense to move simplify="auto" processing into .geojson method

[Kirill888]

See #248 instead