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 bbox validation #167

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ujjwal360
Copy link

small fix to bbox validation allowing x_min (south west longitude) to be greater than x_max (north east longitude) when crossing the anti meridian as per https://datatracker.ietf.org/doc/html/rfc7946#section-5.2

this issue was also reported in #122 and i am encountering the same validation error in https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch

@ujjwal360 ujjwal360 closed this Feb 6, 2025
@ujjwal360 ujjwal360 deleted the fix-antimeridian-bbox-validation branch February 6, 2025 20:32
@ujjwal360 ujjwal360 restored the fix-antimeridian-bbox-validation branch February 6, 2025 20:36
@ujjwal360 ujjwal360 reopened this Feb 6, 2025
@vincentsarago
Copy link
Member

@ujjwal360 thanks for opening (and closing and re-opening 😅) this PR

Could you add some test for this 🙏

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.15%. Comparing base (a927d25) to head (3616d2d).
Report is 81 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
- Coverage   96.59%   96.15%   -0.44%     
==========================================
  Files          25       25              
  Lines         587      598      +11     
==========================================
+ Hits          567      575       +8     
- Misses         20       23       +3     
Flag Coverage Δ
unittests 96.15% <100.00%> (-0.44%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ujjwal360
Copy link
Author

@ujjwal360 thanks for opening (and closing and re-opening 😅) this PR

Could you add some test for this 🙏

thanks for the quick response @vincentsarago, i was having serious second thoughts even though its a small change! 😅 i'll add a test asap

@ujjwal360
Copy link
Author

@vincentsarago hope test looks good

)
# xmin > xmax is permitted when crossing the antimeridian
# https://datatracker.ietf.org/doc/html/rfc7946#section-5.2
if not ((xmax < 0) and (xmin > 0)):
Copy link
Member

@gadomski gadomski Feb 10, 2025

Choose a reason for hiding this comment

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

I don't know if we need/want this condition at all. It's unlikely, but what about a latitude band that's everything except New Zealand? E.g.

image

This would need a bounding box with both xmax and xmin > 0.

Copy link
Author

Choose a reason for hiding this comment

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

it does make sense to remove the


if xmax < xmin:
                raise ValueError(
                    "Maximum longitude must be greater than minimum longitude"
                )

check entirely

for the everything except new zealand bbox, we would have a bounding box like

[ 180, -50, 165, -30]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants