Bounding Box cannot specify a Box which goes over the -180/+180 degree Meridian (v1.3.0)

[vinerich]

Expected Behaviour

A constructed bounding box takes the provided points as is and will not reorder them to specifiy a different box.

Actual Behaviour

The provided points are reorderd to always represent a box fully contained in the longitude ranges -180/+180 and latitude ranges -90/+90. As this is a solution to work with always well defined min/max values after the initialization, it is not what I would expect to happen when I define a BoundingBox.

This is the constructor of the BoundingBox class:

public BoundingBox(double y1, double y2, double x1, double x2) {
        minLon = Math.min(x1, x2);
        maxLon = Math.max(x1, x2);
        minLat = Math.min(y1, y2);
        maxLat = Math.max(y1, y2);
    }

Also this method to expand an existing BoundingBox:

public void expandToInclude(BoundingBox other) {
        if (other.minLon < minLon) {
            minLon = other.minLon;
        }
        if (other.maxLon > maxLon) {
            maxLon = other.maxLon;
        }
        if (other.minLat < minLat) {
            minLat = other.minLat;
        }
        if (other.maxLat > maxLat) {
            maxLat = other.maxLat;
        }
    }

In both cases the points are reorderd to not go over the -180/+180 Meridian.

Also in the class TwoGeoHashBoundingBox:

public TwoGeoHashBoundingBox(GeoHash bottomLeft, GeoHash topRight) {
        if (bottomLeft.significantBits() != topRight.significantBits()) {
            throw new IllegalArgumentException(
                    "Does it make sense to iterate between hashes that have different precisions?");
        }
        this.bottomLeft = GeoHash.fromLongValue(bottomLeft.longValue(), bottomLeft.significantBits());
        this.topRight = GeoHash.fromLongValue(topRight.longValue(), topRight.significantBits());
        this.boundingBox = this.bottomLeft.getBoundingBox();
        this.boundingBox.expandToInclude(this.topRight.getBoundingBox());
    }

The constructor suggests that the Box will be saved as it is defined. With bottomLeft and topRight. But since at the bottom of the constructor expandToInclude is called, the points will be reorder again and maybe the defined box will change.

This whole thing may be related to issue #31 but I'm not sure about this.

Proposed Solution

1. I'm not familiar with the classes and goehashing itself so I'd like to ask if it is save to not do the reordering and take the box as it is defined.
2. Split the defined box into 2 boxes. Splitted at the -180/+180 meridian and to perform 2 seach queries. But I personally don't like that approach, cause it leaves the handling of this case to the user.
3. In extend to 2., throw an Exception when such a box is defined. Atleast the user knows what went wrong and is not tapping in the dark.

In the meanwhile I'll fork and try to work on a fix but idk how much effort I can put into it.

I personally would like solution 1 and maybe I'll come up with a PR. What's your opinion?

[mauhiz]

Hi, I'm interested in this too although in practice working with this meridian is rare. Geohash, by definition, does not cross over this meridian, but when searching neighbour, or looking up geohash for a bounding box, it's required to support the "modulo 360" adjustment.

So, I'd suggest that BoundingBox does not take min/max but northwest/southeast without trying to compare longitudes.

[kungfoo]

It is true that for neighbour queries close to the meridian, this should be the case. I will happily review PRs for that change. I might not have the time to do it myself in the next few days.

On Tue, Nov 12, 2019 at 1:56 AM Vincent PÉRICART notifications@github.com wrote:

Hi, I'm interested in this too although in practice working with this meridian is rare. Geohash, by definition, does not cross over this meridian, but when searching neighbour, or looking up geohash for a bounding box, it's required to support the "modulo 360" adjustment.

So, I'd suggest that BoundingBox does not take min/max but northwest/southeast without trying to compare longitudes.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/issues/38?email_source=notifications&email_token=AAAKAKFTIJBKZ4KEBETTIPDQTH5LPA5CNFSM4JJTU35KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDYU2SA#issuecomment-552684872, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKHINJQIARISFGZNZ53QTH5LPANCNFSM4JJTU35A .

[vinerich]

Hi, I'm interested in this too although in practice working with this meridian is rare. Geohash, by definition, does not cross over this meridian, but when searching neighbour, or looking up geohash for a bounding box, it's required to support the "modulo 360" adjustment.

@mauhiz Even if working with it is rare it should be support or atleast stated that it is not supported. Atm it just results in an "undefined" behaviour if the user doesn't know about this.

So, I'd suggest that BoundingBox does not take min/max but northwest/southeast without trying to compare longitudes.

The changes i proposed in #39 handle exactly this. I wouldn't bother with anyone searching over the poles thats just no possible in my opinion.

It is true that for neighbour queries close to the meridian, this should be the case. I will happily review PRs for that change. I might not have the time to do it myself in the next few days.

@kungfoo Thanks for your offer to review the changes. We use it in a current project so I'd be great if you could get to it in the next week (or maybe 2) then we can just upp the maven version and everything is fine.

Please tell me what you think of the changes proposed by #39. (Also I've not really done much work on other peoples projects so let me know anything I could do better)

[vinerich]

Since this is implemented by now this should be closed.