fix type stability of clipping methods

[rafaqz]

Closes #73

Seems the problem is less complicated that we thought. target should be a constructed trait rather than a trait type mostly GeoInterfaces.jl does this too for the same reason.

I can't see any problems with tuples, the red block is just array allocation.

[asinghvi17]

Should we do the same for apply, for consistency's sake?

[rafaqz]

One reason not to do this in apply is we wouldn't be able to use Union or abstract types. I think that's why I wrote it like that.

That might actually be a problem here too?

There is a hack to get the best of both by putting the types inside Val.

[asinghvi17]

Hmm, that's a good point. How about this:

struct Joint{T} <: GeoInterface.AbstractTrait
end

Joint(args...) = Joint{Union{typeof.(args)...}()

and then you pass in a Joint(GI.PointTrait(), GI.LineStringTrait()) to the type? We can bikeshed the name, but that would be the general principle.

If better for general adoption we could even put this in GeoInterface.

Edit: an interesting name might be MultiTrait for this.

[rafaqz]

@skygering do you have any thoughts on changing this argument from a type to a constructed object?

[skygering]

Okay, so I originally had that. My only concern relates to @asinghvi17's comments in #66. In my opinion, this is more painful. However, I don't see it being that big of a problem given that we eventually will have default MultiTraits and people will only specify if they need one specific type out (or at least that is what I imagine being the biggest use case). We can also have some pre-defined MultiTraits that are the most common and useful.

Basically, I don't love it, but if it improves performance, then I don't think it is that bad. Does it improve runtime given the type instability doesn't propagate?

[skygering]

Hmm, that's a good point. How about this:

struct Joint{T} <: GeoInterface.AbstractTrait
end

Joint(args...) = Joint{Union{typeof.(args)...}()

and then you pass in a Joint(GI.PointTrait(), GI.LineStringTrait()) to the type? We can bikeshed the name, but that would be the general principle.

If better for general adoption we could even put this in GeoInterface.

Edit: an interesting name might be MultiTrait for this.

I like the name MultiTrait for this much more than Joint.

[asinghvi17]

We could always convert a tuple input at the top level, and type info should propagate through that, so I don't see it being too large of a burden? It's also not that much different than typing Union{...} and quite a bit cleaner, IMO.

[rafaqz]

I dont think MultiTraits will work in practice because we lose dispatch, that we keep with Unions

[asinghvi17]

Summary from Slack:

• Acceptable forms of input to target are trait instances (PointTrait()), tuples of trait instances, union types, and multitraits.
• These are all converted to multitraits internally and propagated down.
• MultiTraits are what we dispatch on even if only one trait is needed, by necessity.

[rafaqz]

Ok, this PR is now implementing TraitTarget wrapper everywhere applicable. Its basically Val with a name. Has the same effect as switching to constructed traits.

[asinghvi17]

Can we move these definitions to where the booltypes are now defined, and change the last type from Union to BoolsAsTypes?

[asinghvi17]

I haven't tried this out yet, but did you notice a performance speedup?

[rafaqz]

The same one as swapping to constructed traits, shown in the profile.

apply is still not 100% type stable in all cases but I assume there is something else going on there.