Refactor lake and sea algorithms as GeneralizeWaterAreas #27
Conversation
JuhoErvasti
changed the title
| @dataclass(frozen=True) | ||
| class GeneralizeWaterAreas(BaseAlgorithm): | ||
| """Generalizes polygonal water areas. | ||
|
|
There was a problem hiding this comment.
I'm not sure if some additional description of inputs/outputs/reference data usage are necessary here. I think a comment regarding if reference_data is expected to have some specific data or is optional could be helpful at least.
| reference_data: dict[str, GeoDataFrame], | ||
| ) -> GeoDataFrame: | ||
| if not check_gdf_geometry_type(data, ["Polygon"]): | ||
| msg = "GeneralizeLakes works only with Polygon geometries!" |
There was a problem hiding this comment.
A very small and nitpicky thing, but I've wondered should we try to have a uniform style for error messages. I think I've mainly used period to end the message, but I think exclamation mark works well too. What do you think?
There was a problem hiding this comment.
Regarding the error messages below, is there some purpose why they formatted differently?
There was a problem hiding this comment.
Yes, I think that would be good. No particular reason for the difference, just didn't think too much about it when writing.
I think a uniform style would definitely be good. Maybe for repeated checks (geometry type etc.) we could even define the messages as constants in exception.py, or do you think that's a good idea?
No strong opinion on the style itself. I think either . or ! is fine as long as it's consistent.
| # This source code is licensed under the MIT license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| import tempfile |
There was a problem hiding this comment.
Not sure if it is strictly necessary to follow the import style in the test files, but TemporaryDirectory could be imported here directly. Some other test files seem to have this same issue - if it is an issue in the first place.
There was a problem hiding this comment.
Some tests seem to be missing still, I suppose adding test for invalid input geometry type and invalid shoreline geometry type could suffice? Something that came to my mind since I'm not familiar with this algorithm is whether the shronline parameter should depend on your input data (lakes vs. sea data) in any way?
There was a problem hiding this comment.
The shoreline is used for sea data only, to determine points which shouldn't be smoothed (territorial waters). It's not required for lake data. I guess I should add this to the class docstring.
Opening as a draft since some feedback would be helpful in regards to the test failures. Requires still some changes, particularly handling z values, which #22 is making progress on.
The landcover test is the main issue, bumping GeoPandas and Shapely versions causes it to fail with some major differences in results, and I'm not sure why. I'm not really familiar with the landcover algorithm, so any ideas would be helpful @nmaarnio. I can provide a comparison between the results before the version bump and after, if that'd be helpful.
Description
Old PyQGIS-based lake and sea algorithms are removed and refactored into one algorithm
GeneralizeWaterAreas.core.geometryandexaggerationget some new functions. The algorithm itself works like the QGIS-based one, with one difference in that geometries are not combined, and simplification is done with a coverage simplification, keeping topology intact.Similarly, included is a custom implementation of Chaikin's line smoothing algorithm, which allows skipping the smoothing of certain coordinates, therefore enabling to keep
GeoSeries-wide topology intact and also allows to retain territorial water borders unchanged (for sea parts), which the old version couldn't do.Getting the coverage simplification function required bumping GeoPandas and Shapely versions.
Type of change
Bug fix
New feature
Other
Non-breaking change
Breaking change
Developer checklist