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

Move SpatialOperation from lucene to spatial4j #98

Merged
merged 4 commits into from Apr 3, 2015
Merged

Conversation

ryantxu
Copy link
Contributor

@ryantxu ryantxu commented Apr 2, 2015

Right now the spatial operation code is in the lucene module. This means that libraries require lucene core to know the supported operations. We should have access to the operations from client code

Lets move this to spatial4j

@ryantxu
Copy link
Contributor Author

ryantxu commented Apr 2, 2015

in the next release, we could deprecate the lucene values and have them extend from here

@dsmiley
Copy link
Contributor

dsmiley commented Apr 2, 2015

Where do you see this being used within Spatial4j? Especially SpatialArgs? I suspect your answer is nowhere but your client code happens to have Spatial4j on its classpath but not Lucene spatial, so that is what's actually driving this -- which isn't really convincing. That was also the argument for GeohashUtils being here, which still feels arbitrary; it's got nothing to do with the rest of Spatial4j. It's fundamentally not about shapes. Something could be said about SpatialPredicate (formerly SpatialOperation) specifically though... especially if we want to break out of the limited SpatialRelation options.

@ryantxu
Copy link
Contributor Author

ryantxu commented Apr 2, 2015

Good point about SpatialArgs -- i removed that from this request.

SpatialPredicate does feel like a generally useful client action

@dsmiley
Copy link
Contributor

dsmiley commented Apr 2, 2015

Before this can be moved forward (merged to master) I think it needs to be clear where the Shape API might use this. If it sits there with no other class using it (like GeohashUtils) then why bother?

@ryantxu
Copy link
Contributor Author

ryantxu commented Apr 2, 2015

Isn't this for people trying to use the Shape API? As a Shape consumer, i would WAY rather write:

SpatialPredicate.IsDisjointTo( a, b )
vs
! a.relate(b).intersects()

@dsmiley
Copy link
Contributor

dsmiley commented Apr 2, 2015

Okay +1.

Perhaps some day we might have the Shape API have a method like boolean satisfies(predicate, shapeB) or something like that, so that it could perhaps optimize for just knowing the boolean answer rather than a SpatialRelation which is a more rich answer. I dunno yet what the API might look like -- just throwing that out there. Another approach would have direct predicate methods like isWithin(shapeB) right on the shape but that would lead to a lot more methods.

It's a shame the more rich set of predicates supported by JTS in JtsGeometry is isolated there. Maybe that's somewhat inevitable.

ryantxu added a commit that referenced this pull request Apr 3, 2015
Move SpatialOperation from lucene to spatial4j
@ryantxu ryantxu merged commit 4db6694 into master Apr 3, 2015
@ryantxu
Copy link
Contributor Author

ryantxu commented Apr 3, 2015

It's a shame the more rich set of predicates supported by JTS in JtsGeometry is isolated there. Maybe that's somewhat inevitable.

Yes -- it seems like that is the tradeoff for having JTS as an optional dependency

@ryantxu ryantxu deleted the spatial-operation branch April 3, 2015 05:01
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.

None yet

2 participants