-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Commented out tests #6
Comments
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. |
I'm still pinned to the pre-2.0 version, but note that 2.0 is actually already registered (7af1b0e#commitcomment-99608912). |
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). |
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 :) |
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? |
You are right, such a filter is no longer possible with 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:
I let you check that the new version works as you expected before making a new release. You'll have to stick to the |
As you pointed, the documentation is not up to date. I am working on this. Would you mind if I supply your |
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 |
Sure I'll do that.
Excellent idea, that makes the code clearer! |
I have changed the API of 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. |
As of e93423f the code coverage is better than 75% which helps to find a few bugs. This should address your legitimate concerns. So I will close this issue soon. |
As of version 2.1.0, the test coverage is > 80%. |
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
The text was updated successfully, but these errors were encountered: