search

kungfoo / geohash-java

Implementation of GeoHashes in java. We try to be/stay compliant to the spec, as far as possible.
Other
981 stars 310 forks source link

This provides handling of BoundingBoxes which go over the 180-Meridian #39

Closed vinerich closed 4 years ago

vinerich commented 4 years ago

As dicussed in #38.

My changes could be considered "breaking" to some degree. The thing that's maybe breaking it is a "reordering" of the BoundingBox constructors arguments. Before my changes the order of the latitude and longitude was of no importance and the min/max was used to assign the BoundingBox corners. Since a (as called by @mauhiz) "modulo 360" adjustment needs fixed corner points in the constructor of the BoundingBox i changed the min/max handling to south/north/east/west handling. With this change I also had to rearrange several test cases of yours to match the new specifiication and I suppose this will also happen to some users of this library.

// New constructor with explicit definition of corner points
public BoundingBox(double southLatitude, double northLatitude, double westLongitude, double eastLongitude) {
        if (southLatitude > northLatitude)
            throw new IllegalArgumentException("The southLatitude must not be greater than the northLatitude");

        if (Math.abs(southLatitude) > 90 || Math.abs(northLatitude) > 90 || Math.abs(westLongitude) > 180 || Math.abs(eastLongitude) > 180) {
            throw new IllegalArgumentException("The supplied coordinates are out of range.");
        }

        this.northLatitude = northLatitude;
        this.westLongitude = westLongitude;

        this.southLatitude = southLatitude;
        this.eastLongitude = eastLongitude;

        intersects180Meridian = eastLongitude < westLongitude;
    }

I tried to be compliant to your UpperLeft() and LowerRight() - Methods but renamed them into NorthWest() and SouthEast() for clarity and consistence. I also renamed some public methods to be consistent. You can rename them to the old names I would have no problem with that.

I got one thing I can't get right atm, or atleast I don't really understand what's happening there. The testStepsBetween()-Test in GeoHashTest.java fails. If you could take this @kungfoo I'd greatly appreciate it. Ofc you can delegate the issue to me if my changes are the cause but I don't fully understand what this function is doing atm.

There is a minor change regarding the toString()-Method in the GeoHash-class. The output is now padded to the left with zeros since the GeoHash can have leading zeros.

vinerich commented 4 years ago

There is a second error concerning the BoundingBoxSamplerTests which seems related to the testStepsBetweenError(). I'll look into it and report back.

Edit: Fixed both errors in one. Caused by unconsistency of renaming due to (360-modulo).

kungfoo commented 4 years ago

The fact that you're getting a point is already embedded in the return type, so imho it's superfluous. I know this is not exactly like the rest of the codebase looks because I wrote it a LONG time ago. :)

Nowadays I would even get rid of the get in the method name, so I'd go with northEastCorner() but that is very much not aligned with the rest of the code base.

Le'ts go with getNorthEastCorner() for now....

On Mon, Nov 18, 2019 at 11:38 AM bl4ck notifications@github.com wrote:

@b14ck1 commented on this pull request.

In src/main/java/ch/hsr/geohash/BoundingBox.java https://github.com/kungfoo/geohash-java/pull/39#discussion_r347306857:

}
  • public WGS84Point getUpperLeft() {
  • return new WGS84Point(maxLat, minLon);
  • /**
    • Returns the NorthWestPoint of this BoundingBox as a new Point.
  • *
    • @return
  • */
  • public WGS84Point getNorthWestPoint() {

I'd say its up to you. I liked point at the time of writing cause it clarifies what type will be returned. But that's also specified in the method header so yeah idc.

Corner is probably a little bit more clarifying if you call this method directly from the BoundingBox-object.

Shall we go with northEastCornerPoint()? That would be most clearly for me.

Do you want to emit the get in the method name or did you just forget to write it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/pull/39?email_source=notifications&email_token=AAAKAKCRLSPQFXFJPJUSS33QUJWBJA5CNFSM4JM2723KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCL36MXI#discussion_r347306857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKDD7ZR6V63UGCC4JF3QUJWBJANCNFSM4JM2723A .

vinerich commented 4 years ago

Thanks for the feedback. Screened all classes and renamed. :)

kungfoo commented 4 years ago

Thank you so much, I'll look into it tonight.

On Mon, Nov 18, 2019 at 1:16 PM bl4ck notifications@github.com wrote:

Thanks for the feedback. Screened all classes and renamed. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/pull/39?email_source=notifications&email_token=AAAKAKFYYUI6EFQOOHUSTSTQUKBRHA5CNFSM4JM2723KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEKH3SI#issuecomment-554991049, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKF5WSKB5NLG3PCAGQ3QUKBRHANCNFSM4JM2723A .

vinerich commented 4 years ago

Hey, did you got the time to look into it? Do you have any questions left? We would really look forward to integrate it into our project.

Thanks and best regards :)

vinerich commented 4 years ago

Im sorry for bumping this again. If you got no time at the moment that's totally fine. But since we need the proposed changes for our current project, I'd release my fork by myself to make it available over maven. Is that ok with you, aslong as I mention everyone properly and make it clear that this is just a temporary release?

kungfoo commented 4 years ago

Wait, I totally forgot about this this week. I 'll have a look and release a version with the changes in. Thanks for all the hard work.

On Tue, Dec 10, 2019 at 1:18 PM richwine notifications@github.com wrote:

Im sorry for bumping this again. If you got no time at the moment that's totally fine. But since we need the proposed changes for our current project, I'd release my fork by myself to make it available over maven. Is that ok with you, aslong as I mention everyone properly and make it clear that this is just a temporary release?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/pull/39?email_source=notifications&email_token=AAAKAKHEYIPA6Q65NKXUET3QX6CKLA5CNFSM4JM2723KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPBEFQ#issuecomment-564007446, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKEWR7ICSUEO3TOFEWTQX6CKLANCNFSM4JM2723A .

kungfoo commented 4 years ago

Merged the changes, currently fighting sonatype oss to realease.

On Tue, Dec 10, 2019, 13:32 Silvio Heuberger silvio.heuberger@gmail.com wrote:

Wait, I totally forgot about this this week. I 'll have a look and release a version with the changes in. Thanks for all the hard work.

On Tue, Dec 10, 2019 at 1:18 PM richwine notifications@github.com wrote:

Im sorry for bumping this again. If you got no time at the moment that's totally fine. But since we need the proposed changes for our current project, I'd release my fork by myself to make it available over maven. Is that ok with you, aslong as I mention everyone properly and make it clear that this is just a temporary release?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/pull/39?email_source=notifications&email_token=AAAKAKHEYIPA6Q65NKXUET3QX6CKLA5CNFSM4JM2723KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGPBEFQ#issuecomment-564007446, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKEWR7ICSUEO3TOFEWTQX6CKLANCNFSM4JM2723A .

vinerich commented 4 years ago

Thanks! But still, do not rush it, take your time. As long as you're on it that's fine. Looking forward!!

So no more changes needed? You're fine with the proposed changes?

kungfoo commented 4 years ago

I am still fighting ossrh and maven. :) I have to update all the plugins and build files to support releasing a new version, which is kinda ridiculous, but still...

kungfoo commented 4 years ago

I won the fight. :smile:

You can find 1.4.0 here: https://repo1.maven.org/maven2/ch/hsr/geohash/1.4.0/

vinerich commented 4 years ago

Hey, thank you! I wasn't at work but I'll integrate it tomorrow and have a look!! I hope I didn't annoy you to much. If there are any issues coming up later, feel free to mention me.

kungfoo commented 4 years ago

Yeah, please do let me know how it goes.

On Wed, Dec 18, 2019, 19:27 vinerich notifications@github.com wrote:

Hey, thank you! I wasn't at work but I'll integrate it tomorrow and have a look!! I hope I didn't annoy you to much. If there are any issues coming up later, feel free to mention me.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kungfoo/geohash-java/pull/39?email_source=notifications&email_token=AAAKAKBMDAP64R3RDSLIRR3QZJTSBA5CNFSM4JM2723KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHHBFGA#issuecomment-567153304, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKAKEBK63WNUV3A7KZ3NDQZJTSBANCNFSM4JM2723A .