Improve Spatial Utility like classes [LUCENE-2147]

asfimport commented 14 years ago

DistanceUnits can be improved by giving functionality to the enum, such as being able to convert between different units, and adding tests.
GeoHashUtils can be improved through some code tidying, documentation, and tests.
SpatialConstants allows us to move all constants, such as the radii and circumferences of Earth, to a single consistent location that we can then use throughout the contrib. This also allows us to improve the transparency of calculations done in the contrib, as users of the contrib can easily see the values being used. Currently this issues does not migrate classes to use these constants, that will happen in issues related to the appropriate classes.

Migrated from LUCENE-2147 by Chris Male, resolved Jan 05 2010 Attachments: LUCENE-2147.patch (versions: 5) Linked issues:

3215

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Added patch that implementations described behavior.

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Ideally DistanceUnits would be renamed to DistanceUnit and moved to the new util package, and GeoHashUtils would be moved to the util package as well.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Chris, good stuff so far. I have added a couple of final keywords and prevent some autoboxing in GeoHashUtils as the char / int values should be mainly cached anyway. this prevents a couple of object creations. I also added a testcase which relates to the weird precision issues in #2890 that seem to be gone now - good stuff! -> btw I like that we have only one decode method instead of the precision one.

At the current state I found that the SpatialConstants should be part of DistanceUnits provided as getters. This might change in the future if there are more constants but for now I would rather put them into the enum as this enforces consistency for distance units.

thoughts?

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Hi,

Yeah you are right about DistanceUnits using the SpatialConstants. I would like to leave SpatialConstants in as it gives us a single place to manage these values, particularly if the values are dependent on eachother (for example the circumference should really depend on the radius). However adding to DistanceUnits getEarthRadius() and getEarthCircumference() would make alot of sense in the current environment. These could then simply pull their values from the SpatialConstants.

I am fine with removing SpatialConstants if you feel this is overkill.

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Fixed bug in the patch which assigned earthCircumference to earthRadius in the DistanceUnits constructor.
Added javadoc to DistanceUnits constructor for new parameters
Tidied some of the other javadoc

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

chris this looks good I feel that we are pretty close to commit this. I will wait another 2 days, review again and announce the commit.

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Improved javadoc on DistanceUnits.convert so its a little clearer.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Chris, this seems to be ready to be committed soon. I removed the "flux" warnings in the class JavaDocs, converted the tests to junit 4 and added a CHANGES.TXT notice to make it ready to be committed.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Since this is the first issue which comes near to be committed some questions arise from my side if we should mark the new API as experimental like the function API in o.a.l.s.function. I think it would make sense to keep a warning that contrib/spatial might slightly change in the future. On the other hand we should try to put more confidence into contrib/spatial for more user acceptance. I currently work for customers that moved away from spatial due to its early stage and "flux" warnings which is quite understandable though. I would like to hear other opinions regarding this topic - especially opinions of more experienced committers would be appreciated.

asfimport commented 14 years ago

Grant Ingersoll (@gsingers) (migrated from JIRA)

I'd say that we remove the flux warnings, but instead put a note in the top level that since this is a contrib module, it will not adhere to Lucene core's strict back compat. policy.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

I'd say that we remove the flux warnings, but instead put a note in the top level that since this is a contrib module, it will not adhere to Lucene core's strict back compat. policy.

that sounds good, I will put it into a package.html doc and will also add a readme to the project itself.

I think this issue is good to go. I will commit this is a few days if nobody objects.

asfimport commented 14 years ago

Chris Male (migrated from JIRA)

Okay, I will remove them from the other patches as well and update them over the next few days.

asfimport commented 14 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

Committed in revision 896240

Thanks Chris

apache / lucene

Improve Spatial Utility like classes [LUCENE-2147] #3223

3215