rafaqz / Extents.jl

A shared Extent object for Julia spatial data, with DE-9IM spatial predicates
MIT License
4 stars 3 forks source link

add more Base and utility methods #3

Closed rafaqz closed 2 years ago

rafaqz commented 2 years ago

This PR adds utility methods:

For this to work well, and for other uses in Rasters.jl/DimensionalData.jl, I also added Base methods and did some work on the type to make Extent more similar to NamedTuple.

Use of Val in keys is so all the named tuple indexing compiles away - which gives 15ns union and intersect rather than 300ns.

@evetion if you could have a look over this that would be great

evetion commented 2 years ago

You could rename intersect to intersection, that makes it more distinct and follows convention.

rafaqz commented 2 years ago

Yeah I just saw that in ArchGDAL. The problem is the base Julia convention is intersect.

evetion commented 2 years ago

All new functions could have an optional strict argument, that's false by default, indicating whether the result only operates on shared dimensions or not?

rafaqz commented 2 years ago

Is the idea to throw an error if they dont match when strict = true?

evetion commented 2 years ago

Hmm, maybe just strict isn't enough if we want to be verbose. In the case of mismatching dimensions (but at least 1 overlap)

I dislike the error, so maybe only give back nothing/false in case of strict? The union feels odd here, because the name implies things about the resolving of keys already.

What's the behaviour if none of the keys overlap?

rafaqz commented 2 years ago

Yeah union is wierd. Like should we even include non shared dimensions in the default output? What does a union mean accross dimensions?

Probably returning nothing is best for strict, you're right.

rafaqz commented 2 years ago

Ok mostly done these. My main remaining question is if union returns all dimensions available or just the shared dimensions. The name does kind of suggests it will return all - but currently it just returns shared dimensions

evetion commented 2 years ago

That's seems fine, the name is slightly confusing, but it's just about the extents, not about dimensions.

evetion commented 2 years ago

I've fixed a test for union (returning nothing instead of ()) for no shared dimensions and added docstrings for the strict keyword. Feel free to merge if you agree.