Skip to content
This repository has been archived by the owner on Dec 11, 2022. It is now read-only.

Fix cropping when omitting a limit #18

Merged
merged 8 commits into from
Apr 10, 2020
Merged

Fix cropping when omitting a limit #18

merged 8 commits into from
Apr 10, 2020

Conversation

gabrieldansereau
Copy link
Member

@gabrieldansereau gabrieldansereau commented Apr 9, 2020

Cropping a layer while omitting to specify one of the limits did not work properly. For example:

julia> temp = worldclim(1);
julia> temp[left = -145.0, right = -50.0, top = 75.0, bottom = 20.0];
julia> temp[left = -145.0, right = -50.0, top = 75.0];
ERROR: MethodError: no method matching #getindex#2(::Float64, ::Float64, ::Float64, ::Nothing, ::typeof(getindex), ::SimpleSDMPredictor{Float64})
Closest candidates are:
  #getindex#2(::K, ::K, ::K, ::K, ::typeof(getindex), ::T) where {T<:SimpleSDMLayer, K<:Union{Nothing, AbstractFloat}} at /home/gdansereau/github/SimpleSDMLayers.jl/src/lib/overloads.jl:166
Stacktrace:
 [1] top-level scope at none:0

I fixed it with two tweaks to the getindex overload (the match_longitude calls and the types).

While at it, I also added an overload to crop based on a NamedTuple. I think it's useful as it allows to crop with a variable, which can be reused easily.

coords = (left = 145.0, right = -50.0, top = 75.0, bottom = 20.0)
temp[coords]

I've added tests for both of these.

imin = isnothing(left) ? p.left : match_longitude(p, left)
jmax = isnothing(top) ? p.top : match_latitude(p, top)
jmin = isnothing(bottom) ? p.bottom : match_latitude(p, bottom)
function Base.getindex(p::T; left::Union{N,A}=nothing, right::Union{N,A}=nothing, top::Union{N,A}=nothing, bottom::Union{N,A}=nothing) where {T <: SimpleSDMLayer, N <: Nothing, A <: AbstractFloat}
Copy link
Member Author

@gabrieldansereau gabrieldansereau Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to make the types work? They have to be "independent", so that one can be Nothing while the others are AbstractFloat. The previous code only worked if they were all Nothing or all AbstractFloat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can always not be specific about the types here, since there is an isnothing on one side, and the getindex measures do not have methods for anything other than a float in this case... would it fix the tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it already works like this. I was just wondering if there was a cleaner way to call the types.
It works the same if we're not specific about them, as match_longitude is specific for floats.

@tpoisot
Copy link
Member

tpoisot commented Apr 10, 2020

I think this should simplify the declaration substantially

@gabrieldansereau
Copy link
Member Author

Yes, that's great!

@tpoisot tpoisot merged commit 17db192 into master Apr 10, 2020
@gabrieldansereau gabrieldansereau deleted the fix/cropping-limit branch April 10, 2020 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants