Fix broken tests with latest version

[juliohm]

We refactored the Chain interface and some of the tests are now breaking. We need to take a closer look and fix.

[juliohm]

@ErickChacon any chance you can take a look at this? I've commented some tests that are failing and am not sure if they are failing because of an issue in our side or an issue in one of the backend packages (Shapefile.jl, GeoJSON.jl, ...).

[ErickChacon]

Given that our interface has changed, it is probably related to our side. I will check it.

[juliohm]

We replaced Chain by Rope + Ring. I modified the tochain and topolygon function in conversion.jl. Moving forward we have the "closedness" of a chain as part of the type.

[ErickChacon]

After some investigation. I noted that the error occurs when trying to write a Multi Rope to GeoJSON.jl. For example, when writing a MultiRope is saved as a Polygon and then there is an error when loading it used in GeoTables.jl.

https://github.com/JuliaGeo/GeoJSON.jl/blob/b9825e924eaed4856bcaa7cdc55138e185a7a477/src/io.jl#L82-L83

There has been a change on April about those lines. It might be related to that, but I am not completely sure. I will try to make some time to check it in more detail.

[juliohm]

Thank you @ErickChacon for nailing down the issue. So apparently we discovered a bug in GeoJSON.jl?

I am really enjoying our new Rope/Ring types. They are helping with the identification of bugs downstream already.

[juliohm]

@ErickChacon I replaced the check first(points) == last(points) in our tochain function by GI.isclosed(geom) to make sure that we are not missing anything. The issue remains.

@eliascarv pointed out that this issue may be related to the fact that GeoJSON.jl saves as Float32 and then coordinates that used to be different in Float64 became equal in single precision.

[ErickChacon]

I do not think it is a problem related to the float type. I think is the design of saving multilinestrings as Polygons in GeoJSON.jl. I just want to check why this was not a problem before, so I am completely sure and open a issue in GeoJSON.jl

[ErickChacon]

I think I know what's happening:

1. It was already an existing bug, we were not able to catch it because before we were reading lazily our saving data, we did not need to transform the geometries to Meshes.jl, so we were not checking if the geometries were valid.
2. We starting to see the bug when introducing the lazy argument. Now, by default, we already convert the geometries, so we get errors for invalid geometries.
3. The previous PolyArea implementation used to send clear messages when the chains were not valid thanks to the lines: https://github.com/JuliaGeometry/Meshes.jl/blob/7e9ee079d0581e4b1f2f6f4772170a9b00ad46fe/src/polytopes/polyarea.jl#L29-L30

For example, the previous implementation would show the following message:

# ERROR: AssertionError: invalid outer chain
# Stacktrace:
#   [1] PolyArea{2, Float32, Chain{2, Float32, Vector{Point2f}}}(outer::Chain{2, Float32, Vector{Point2f}}, inners::Vector{Any}, fix::Bool)

I will create a MWE and start a issue in GeoJSON.jl.

[ErickChacon]

Continuing with the previous explanation. The error is clear now:

ERROR: MethodError: no method matching PolyArea(::Rope{2, Float64, Vector{Point2}})

Meaning that Meshes.jl can not construct the polygon saved by GeoJSON because first and last points are not the same. Again, this is definitely an issue with GeoJSON.jl because is saving an invalid polygon.

[juliohm]

Hi @ErickChacon any advance on the GeoJSON.jl side?