Skip to content
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

Fix clipping with exact coordinates #144

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Fix clipping with exact coordinates #144

merged 2 commits into from
Feb 15, 2023

Conversation

gabrieldansereau
Copy link
Member

@gabrieldansereau gabrieldansereau commented Feb 15, 2023

Fixes #143

This fixes the coordinate clipping issue with the values I tested (as shown in the example below), but I'm not sure if it could cause other issues and I don't think it solves it for all values.

There are two approximation issues at play here. One is in _match_latitude and _match_longitude and causes clip(layer; top=56.0).top to return the wrong coordinate. My fix there is a bit ugly as I'm not sure what's going in those functions. It is sufficient to fix the issues when clipping with top and left in my example, but not with bottom and right.

The 2nd approximation issue occurs in clip itself, for instance with clip(layer; bottom=64.0. Here the index returned by _match_latitude is correct, but subtracting the stride returns 63.9999999999999. I used isapprox and round to fix it when trying to clip with natural numbers (which should be the most common case to encounter this bug) but I didn't see how to fix it for all coordinates that would be exactly on the boundary without calling isapprox on all of them.

The full example:

julia> using SpeciesDistributionToolkit

julia> spatialrange = (left = -145.0, right = -50.0, bottom = 40.0, top = 89.0);

julia> layer = SimpleSDMPredictor(RasterData(WorldClim2, BioClim); spatialrange...);

julia> clip(layer; top=56.0).top
56.0

julia> clip(layer; top=56.0 - 0.0000001).top
56.0

julia> clip(layer; top=56.0 + 0.0000001).top
56.16666666666667

julia> exact_lats = (spatialrange.bottom+1.0):1.0:(spatialrange.top - 1.0)
41.0:1.0:88.0

julia> exact_lons = (spatialrange.left+1.0):1.0:(spatialrange.right - 1.0)
-144.0:1.0:-51.0

julia> all(isinteger.([clip(layer; top=l).top for l in exact_lats]))
true

julia> all(isinteger.([clip(layer; bottom=l).bottom for l in exact_lats]))
true

julia> all(isinteger.([clip(layer; right=l).right for l in exact_lons]))
true

julia> all(isinteger.([clip(layer; left=l).left for l in exact_lons]))
true

@github-actions
Copy link
Contributor

@codecov-commenter
Copy link

Codecov Report

Base: 48.53% // Head: 41.01% // Decreases project coverage by -7.52% ⚠️

Coverage data is based on head (b21da36) compared to base (965c23c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   48.53%   41.01%   -7.52%     
==========================================
  Files          46       35      -11     
  Lines        1430     1141     -289     
==========================================
- Hits          694      468     -226     
+ Misses        736      673      -63     
Flag Coverage Δ
unittests 41.01% <100.00%> (-7.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
SimpleSDMLayers/src/lib/clip.jl 100.00% <100.00%> (ø)
SimpleSDMLayers/src/lib/coordinateconversion.jl 100.00% <100.00%> (ø)
...impleSDMDatasets/src/providers/CHELSA/chelsa_v1.jl
...mpleSDMDatasets/src/providers/EarthEnv/earthenv.jl
SimpleSDMDatasets/src/types/specifiers.jl
...MDatasets/src/providers/CHELSA/chelsa_future_v2.jl
SimpleSDMDatasets/src/downloader.jl
...ets/src/providers/WorldClim/worldclim_future_v2.jl
SimpleSDMDatasets/src/keychecker.jl
...MDatasets/src/providers/CHELSA/chelsa_future_v1.jl
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tpoisot tpoisot merged commit 9eda200 into main Feb 15, 2023
@tpoisot tpoisot deleted the bug/clip-exact-coords branch February 15, 2023 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Wrong coordinates returned when clipping with exact coordinates
3 participants