csiro-coasts / emsarray

xarray extension that supports EMS model formats
BSD 3-Clause "New" or "Revised" License
13 stars 2 forks source link

Test polygons are valid after constructing them #156

Closed mx-moth closed 2 months ago

mx-moth commented 2 months ago

This came about after #154 which was generating invalid polygons which were not picked up. This PR introduces a post-processing step on the Convention.polygons property which checks the polygons to ensure they are valid polygons. Invalid polygons will spoil other operations so it is good to be certain.

This change effectively requires any implementing conventions to rename their polygon property to a _make_polygons() method. The base Convention class now includes a polygon property which calls this new method and post-processes the results. ~A new 'hotfix' concept is being trialled here which can adapt a Convention with a polygon property and without a _make_polygons() method to the new interface. This is novel but honestly there is only one plugin for emsarray so possibly this is unnecessary extra complications. Still, it is a problem I had been mulling over for some time and I am happy I found a workable solution if only for my own education!~

Checking for polygon validity will slow things down, but not by a huge amount. The additional safety is worth it. If this safety check becomes burdensome in the future we could consider adding a 'strict' / 'non-strict' mode which enables / disables these sorts checks. In 'non-strict' mode loading datasets would be faster but any errors in the geometry would not be picked up, potentially giving invalid results or otherwise causing errors.