LocalFilters.jl icon indicating copy to clipboard operation
LocalFilters.jl copied to clipboard

Commented out tests

Open evetion opened this issue 1 year ago • 10 comments

Is it correct that since 2.0 most of the API tests are commented out? See https://github.com/emmt/LocalFilters.jl/blob/master/test/runtests.jl#L20-L133 and https://github.com/emmt/LocalFilters.jl/blob/master/test/runtests.jl#L303-L610

evetion avatar May 24 '23 07:05 evetion

Yes the 2.0 branch is still a work in progress. It is not yet registered and I have disabled lots of tests because some editing is needed before let them run again. Please, let me know if you are using API 2.0.

emmt avatar May 25 '23 22:05 emmt

I'm still pinned to the pre-2.0 version, but note that 2.0 is actually already registered (https://github.com/emmt/LocalFilters.jl/commit/7af1b0eba0e644b35dc95c5f204916b1bf893634#commitcomment-99608912).

evetion avatar May 26 '23 07:05 evetion

Oops you are right. I need to do some cleanup. I have also noticed that the doc. is the old one...

BTW why are you still pinned to version 1? Version 2 introduces many changes under the hood, but relatively few for the end-user (at high level) and some methods are mich faster. Moving from LocalFilters v1 to v2 may require a few edits (see these guidelines).

emmt avatar May 26 '23 11:05 emmt

BTW why are you still pinned to version 1? Version 2 introduces many changes under the hood, but relatively few for the end-user (at high level) and some methods are much faster. Moving from LocalFilters v1 to v2 may require a few edits (see these guidelines).

I always use pin/compat my code/packages, I just didn't have the time to update to 2.0 yet. My compliments on the migration guide :)

evetion avatar May 27 '23 13:05 evetion

Today I tried to update to LocalFilters v2, but I think some of the functionality I use extensively in v1 is gone. For example, given the following function:

"""Largest difference to centre cell."""
function roughness(dem::AbstractMatrix{<:Real})
    dst = copy(dem)

    initial(a) = (zero(a), a)
    update(v, a, _) = (max(v[1], abs(a - v[2])), v[2])
    store!(d, i, v) = @inbounds d[i] = v[1]

    return localfilter!(dst, dem, 3, initial, update, store!)
end

I use initial to get the actual value of a cell, and use it to compare to all other values. How would this pattern work in v2?

evetion avatar Mar 27 '24 04:03 evetion

You are right, such a filter is no longer possible with initial being the initial value of the state variable of the local filter. I have fixed this in cb364472f902c6d8b900afe073e57061bf5db38c so that initial can also be a function. Your filter would then write:

function roughness(dem::AbstractMatrix{<:Real}, B=3)
    initial(a) = (zero(a), a)
    update(v, a, _) = (max(v[1], abs(a - v[2])), v[2])
    final(v) = v[1]
    return localfilter!(similar(dst), dem, B, initial, update, final)
end

Notes:

  • You do not need to copy the source, similar is sufficient.
  • I have added the possibility to use another neighborhood B.
  • In version 2 of LocalFilters, the update! method has been replaced by a simpler final one which extracts the result from the state variable.
  • You may consider reading the top of NEWS.md for general rules to convert to the new version of LocalFilters. Let me known if these are not clear enough.

I let you check that the new version works as you expected before making a new release. You'll have to stick to the master version in the mean time...

emmt avatar Mar 27 '24 10:03 emmt

As you pointed, the documentation is not up to date. I am working on this.

Would you mind if I supply your roughness function as an example of use?

emmt avatar Mar 28 '24 08:03 emmt

Thanks for the quick replies and fixes! 🎉 I will test whether the master branch works later this weekend.

In the meantime, feel free to use the example, you could also link to the package that uses it: https://github.com/Deltares/Geomorphometry.jl/blob/main/src/terrain.jl (MIT license).

You could consider using a namedtuple for readability:

function roughness(dem::AbstractMatrix{<:Real}, B=3)
    initial(a) = (;result=zero(a), center=a)
    update(v, a, _) = (;result=max(v.result, abs(a - v.center)), center=v.center)
    final(v) = v.result
    return localfilter!(similar(dem), dem, B, initial, update, final)
end

evetion avatar Mar 28 '24 10:03 evetion

In the meantime, feel free to use the example, you could also link to the package that uses it: https://github.com/Deltares/Geomorphometry.jl/blob/main/src/terrain.jl (MIT license).

Sure I'll do that.

You could consider using a namedtuple for readability:

Excellent idea, that makes the code clearer!

emmt avatar Mar 30 '24 08:03 emmt

I have changed the API of LocalFilters as detailed in the top of NEWS.md. The last master version should work for you. Almost all changes in the public API are backward compatible with version 2.0.0.

I have mostly done rewriting/updating the doc which is built by GitHub Action as you can see here.

Next step before an official 2.1.0 (not 2.0.1 due to the substancially augmented API) release is to re-activate tests.

emmt avatar Apr 02 '24 18:04 emmt