getXMin et al.

[mathieu17g]

Should fix #93

Includes:

• libgeos_api.jl regeneration
• adaptation of setPrecision methods and test to new Cenum type definition GEOSPrecisionRules
• commented code for future GEOSGeom_getExtent_r wrapping

[evetion]

The regenerated API part will clash with the competing #94.

[mathieu17g]

@evetion

The regenerated API part will clash with the competing #94.

The merge should not be problematic, since @visr and I did the same actions described in #93 (comment)

[mathieu17g]

In commit 4fae37b, I have:

1. changed the use of @eval to @generated
2. exported the new getXMin et al. functions

Concerning the change from @eval to @generated
I was able to add docstrings to getXMin et al. with @eval through @doc macro on the 1st symbol :Point, but the use of @generated is neater.

@visr if it suits you, I will make the change for the other functions in geos_operations.jl and add documentation

[visr]

but the use of generated is neater.

I'd prefer to avoid generated functions if at all possible. I cannot properly explain because I don't fully understand, but in the Julia ecosystem I think most ofter their use is avoided if possible, because of other downsides they bring.

Even for eval I'd argue the same thing, avoid using it unless it brings significant benefits. Even as a maintainer I get confused, when I cannot find a method, only to remember it is being created by eval. Adding a one line method f(geom) = f(geom.ptr) directly below the other f is easier to read in my opinion. However, consistency with the rest of the code is more important, which uses eval. If we want to change that it should be in a separate PR for all functions.

[mathieu17g]

I'd prefer to avoid generated functions if at all possible. I cannot properly explain because I don't fully understand, but in the Julia ecosystem I think most ofter their use is avoided if possible, because of other downsides they bring.

It's a pity for me, I'm very fond of it :-)

The issue I was trying to solve is to add documentation to geos operations functions.
Since the rest of the code uses @eval for geos operation functions adding a docstring would require something like:

for (i, geom) in enumerate((:Point, :MultiPoint, :LineString, :MultiLineString, :LinearRing, :Polygon, :MultiPolygon, :GeometryCollection))
    @eval begin
        $i == 1 && @doc """
            getXMin(geom, context=_context)

        Finds the minimum X value in the geometry
        """
        getXMin(obj::$geom, context::GEOSContext = _context) = getXMin(obj.ptr, context)
    end
end

or

for (i, geom) in enumerate((:Point, :MultiPoint, :LineString, :MultiLineString, :LinearRing, :Polygon, :MultiPolygon, :GeometryCollection))
    @eval getXMin(obj::$geom, context::GEOSContext = _context) = getXMin(obj.ptr, context)
    @doc """
        getXMin(geom, context=_context)

    Finds the minimum X value in the geometry
    """ getXMin
end

I find them both more cluttered than:

"""
    getXMin(geom, context=_context)

Finds the minimum X value in the geometry
"""
@generated function getXMin(geom::T, context::GEOSContext = _context) where {T<:UNION_ALL_GEOMTYPES}
    return :(getXMin(geom.ptr, context))
end

I understand you suggest to implement something unfactored like:

"""
    getXMin(geom, context=_context)

Finds the minimum X value in the geometry
"""
getXMin(obj::Point, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::MultiPoint, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::LineString, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::MultiLineString, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::LinearRing, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::Polygon, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::MultiPolygon, context::GEOSContext = _context) = getXMin(obj.ptr, context)
getXMin(obj::GeometryCollection, context::GEOSContext = _context) = getXMin(obj.ptr, context)

I have the feeling that it would be error prone and tedious to implement for all functions.

Is it what you are aiming at or is it something different ?

[visr]

I don't understand why we need to automatically create docstrings? Docstrings are retrieved for functions, not specific methods. So we can add a single docstring to the manually written getXMin(ptr::GEOSGeom method.

So for this PR, to avoid including too many changes at once, I'd suggest to go with the eval strategy like the rest of the code.

I understand you suggest to implement something unfactored like:

No that indeed looks tedious. But as you can see it is all the same code. So similarly like you did in #102, we can define

Geometry = Union{
    Point,
    MultiPoint,
    LineString,
    MultiLineString,
    LinearRing,
    Polygon,
    MultiPolygon,
    GeometryCollection,
}

getXMin(obj::Geometry, context::GEOSContext = _context) = getXMin(obj.ptr, context)

[mathieu17g]

Geometry = Union{
    Point,
    MultiPoint,
    LineString,
    MultiLineString,
    LinearRing,
    Polygon,
    MultiPolygon,
    GeometryCollection,
}

getXMin(obj::Geometry, context::GEOSContext = _context) = getXMin(obj.ptr, context)

That's perfect ! I have checked the specialization with (@which getXMin(...)).specializations. It does specialize. I had forgotten that Julia specialize on members of a Union type. Thanks !

I will implement the @eval form in this PR and come back with another one to move to the simpler code you found.

Concerning my comment #95 (comment), shall I move the set setPrecision testing to test_geos_operations since it uses geos operation form of setPrecision ?

Concerning my comment #95 (comment), shall I remove the destroying part of testing ?

[visr]

Thanks, and sorry for the long wait! I created #105 as a reminder for replacing the evaled functions.