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 buffer function #17

Closed alex-s-gardner closed 11 months ago

rafaqz commented 11 months ago

Have your checked this with @btime ? It should only take a few nanoseconds. Checking with @profview may also show some problems.

alex-s-gardner commented 11 months ago

Note that buffer only works if coordinates are sorted: https://github.com/rafaqz/Extents.jl/pull/6

rafaqz commented 11 months ago

This is an open question because reversed can mean crossing the date line or similar.

Is it wrong in that case?

alex-s-gardner commented 11 months ago

This is an open question because reversed can mean crossing the date line or similar.

I don't think any of the Extents math will work if you cross the date line.

alex-s-gardner commented 11 months ago

Execute times seem reasonable

julia> c = Extent(X=(0.1, 0.5), Y=(1.0, 2.0), Ti=(DateTime(2000, 1, 1), DateTime(2020, 1, 1)));

julia> @btime Extents.buffer(c, (X=2, Y=2))
  16.741 ns (1 allocation: 64 bytes)
Extent(X = (-1.9, 2.5), Y = (-1.0, 4.0), Ti = (DateTime("2000-01-01T00:00:00"), DateTime("2020-01-01T00:00:00")))

julia> @btime Extents.buffer(c, (X=2, Y=2, Ti=Year(1)))
  912.028 ns (36 allocations: 1.41 KiB)
rafaqz commented 11 months ago

Try to use $ on the variables... any allocation here is not reasonable

alex-s-gardner commented 11 months ago

Try to use $ on the variables... any allocation here is not reasonable

no allocation for X/Y but there is for DateTime

julia> @btime Extents.buffer($c, (X=2, Y=2))
  1.334 ns (0 allocations: 0 bytes)
Extent(X = (-1.9, 2.5), Y = (-1.0, 4.0), Ti = (DateTime("2000-01-01T00:00:00"), DateTime("2020-01-01T00:00:00")))

julia> @btime Extents.buffer($c, (X=2, Y=2, Ti=Year(1)))
  876.647 ns (36 allocations: 1.41 KiB)
Extent(X = (-1.9, 2.5), Y = (-1.0, 4.0), Ti = (DateTime("1999-01-01T00:00:00"), DateTime("2021-01-01T00:00:00")))
rafaqz commented 11 months ago

1.3ns seems right ;)

But yes bit of a problem with DateTime, probably worth a look with @profview and Chulhu.@descend to see what the type instability is from.

Possibly its' the mixed types? Try to use map(+, ext[k], (-buff[k], +buff[k])) instead of the broadcast, map is type stable with mixed tuples.

alex-s-gardner commented 11 months ago

Possibly its' the mixed types? Try to use map(+, ext[k], (-buff[k], +buff[k])) instead of the broadcast, map is type stable with mixed tuples.

this cuts the time in half but we still have considerable allocations

buff = (X=2, Y=2, Ti=Year(1))
@btime Extents.buffer($c, $buff)
567.027 ns (30 allocations: 1.12 KiB)
alex-s-gardner commented 11 months ago

DateTime alone is not the issue:


julia> @btime DateTime(2000, 1, 1) - Year(1)

  0.750 ns (0 allocations: 0 bytes)
1999-01-01T00:00:00

julia> Ti = DateTime(2000, 1, 1);

julia> @btime $Ti - Year(1)
  22.967 ns (0 allocations: 0 bytes)
1999-01-01T00:00:00

julia> @btime map(+, (DateTime(2000, 1, 1),  DateTime(2020, 1, 1)),  (Year(1), - Year(1)))
  0.750 ns (0 allocations: 0 bytes)
(DateTime("2001-01-01T00:00:00"), DateTime("2019-01-01T00:00:00"))

julia> Ti = (DateTime(2000, 1, 1), DateTime(2020, 1, 1));

julia> buff = (Year(1), -Year(1));

julia> @btime map(+, $Ti, $buff)
  49.096 ns (0 allocations: 0 bytes)
(DateTime("2001-01-01T00:00:00"), DateTime("2019-01-01T00:00:00"))
rafaqz commented 11 months ago

Ok looks like k is not constant propagating.

Try Base.@assume_effects :foldable buffer(...

Otherwise we need to manually force K as Val{K}() by doing map(map(Val, K)) do k

then unwrap it with ext[_unwrap(k)].

where

_unwrap(::Val{K}) where K = K
alex-s-gardner commented 11 months ago

Try Base.@assume_effects :foldable buffer(...

no luck with Base.@assume_effects

julia> c = Extent(X=(0.1, 0.5), Y=(1.0, 2.0), Ti=(DateTime(2000, 1, 1), DateTime(2020, 1, 1)));

julia> buff = (X=2, Y=2, Ti=Year(1));

julia> @btime Extents.buffer($c, $buff)
  568.842 ns (30 allocations: 1.12 KiB)
Extent(X = (-1.9, 2.5), Y = (-1.0, 4.0), Ti = (DateTime("1999-01-01T00:00:00"), DateTime("2021-01-01T00:00:00")))
rafaqz commented 11 months ago

Ok you need to force K to constprop with Val as above

Its a nice trick to know ;)

alex-s-gardner commented 11 months ago

Otherwise we need to manually force K as Val{K}() by doing map(map(Val, K)) do k

then unwrap it with ext[_unwrap(k)].

where

_unwrap(::Val{K}) where K = K

I'm having trouble following? It's not clear to me what Val is in this case.

rafaqz commented 11 months ago

Val is a singleton type with one parameter defined in base to do this stuff

(Replace map(K) do k with map(map(Val, K)) do k and all k with _unwrap(k))

Then add that _unwrap function below

codecov-commenter commented 11 months ago

Codecov Report

Merging #17 (a254411) into main (ce6eecd) will increase coverage by 0.08%. The diff coverage is 85.71%.

:exclamation: Current head a254411 differs from pull request most recent head 5938039. Consider uploading reports for the commit 5938039 to get more accurate results

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   84.31%   84.40%   +0.08%     
==========================================
  Files           1        1              
  Lines         102      109       +7     
==========================================
+ Hits           86       92       +6     
- Misses         16       17       +1     
Files Coverage Δ
src/Extents.jl 84.40% <85.71%> (+0.08%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

alex-s-gardner commented 11 months ago

OK, that's like magic to me... now I need to go try to understand why that works.

julia> @btime Extents.buffer($c, $buff)
  45.542 ns (0 allocations: 0 bytes)
Extent(X = (-1.9, 2.5), Y = (-1.0, 4.0), Ti = (DateTime("1999-01-01T00:00:00"), DateTime("2021-01-01T00:00:00")))
rafaqz commented 11 months ago

Its forcing to compile a separate method for each call to the anonymous function (because Val{:a} is a different type to Val{:b}) where the NameTuple key is known at compile time and const props so the return type is also known.

Otherwise k is just a runtime Symbol - and indexing the NamedTuple has a Union return type.

rafaqz commented 11 months ago

Ok seems there was already an _unwrap function, and the tests need the Dates module in Project.toml

rafaqz commented 11 months ago

Thanks!

alex-s-gardner commented 11 months ago

@rafaqz tank you for the hand holding!

rafaqz commented 11 months ago

No worries, I do enjoy setting unreasonably high performance standards 😂