MapProjection: relax coordinate checks/make them configurable

Tim Englich added a comment - 20/Mar/08 5:22 AM Hi, we have got the same Problems with the Bounds check using GeoTools in our Application. It would be very usefull if there will be a new hint to prevent throwing an Exeption in that case.

Martin Desruisseaux added a comment - 20/Mar/08 5:37 AM We have a choice between: * Disable the exception when some hint is provided. * Disable inconditionnaly the exception and replace them by a warning to be logged only once (or few time). On implementation point of view, logging a warning would be simplier than handling a hint. On a conceptual point of view, it may be reasonable too (but I'm not sure). For example a longitude outside the -180/+180° range is not necessary wrong, since 190° is equivalent to -170°. There is a risk that the projection behave badly for those value, but they usually don't since every trigonometric functions (sin, cos, etc.) used by map projections have correct mathematical behavior with those values. The rational behind the coordinate range check was based on the assumption that a out-of-range values are probably user errors and we want (as a debugging help) notify the users about that, but this is not always true. Furthermore for some projections like Mercator, the threshold where we consider that a value is "out-of-range" is somewhat arbitrary - the case is really not as clear as the "missing bursa wolf parameters" case. For example in the Mercator case we throw an exception when a point is too close to a pole. But Double.POSITIVE_INFINITY would be a right answer from a mathematical point of view - it is just that we assume that most renderers would not behave well with such value, so we thrown an exception at the projection step on the basis that error detected early are easier to fix. But how could the Mercator projection be sure that it is a user error? For all those reasons, I'm tempted by logging. In such case I suggest to add the following members in MapProjection: protected static final Logger LOGGER; (if not already there) protected (or private?) boolean verifyCoordinateRanges; maybe some convenience methods to be determined. And replace the exceptions by a warning logged only the first time it occurs for a given MapProjection instance (we would set verifyCoordinateRanges to false after the logging). Because MapProjection are included in ProjectedCRS instances, and because the later are usually cached (when returned by CRS.decode(...)), in most case it will results in only one warning per EPSG code. Do you think it could be a reasonable compromise?

Andrea Aime added a comment - 20/Mar/08 7:11 AM Martin, looking into this I noticed that there are two extra functions, MapProjection.ensureLatitudeInRange, MapProject.ensureLongitudeInRage, that are widely used by projection classes and that would throw and exception when the coords are out of range. The check is a little different than the one done in MapProjection.transform since no tolerance is used. Should I change those methods to just log as well?

Martin Desruisseaux added a comment - 20/Mar/08 7:16 AM It depends... If there are used for checking constructor argument, I suggest to keep them as is. If they are used for checking transform input/output values, I suggest to replace the exception by the warning. If they are used for both, I suggest to split in two methods: one for argument check in constructor (throwing the exception) and one for checking the transform input/output values (just log a warning).

Andrea Aime added a comment - 20/Mar/08 9:14 AM Checked, only used in constructors. Better leave them as they are, very much agreed.

Andrea Aime added a comment - 20/Mar/08 9:38 AM Fixed in gt2 2.4.x and trunk. For reference, the commit was made on 2.4.x at r29680 (should Jody need to backport this on trunk). For the record, this fix allows us to properly overlay data with google maps one even when extremely zoomed out (when google enters in map repeating mode like in http://maps.google.com/?ie=UTF8&ll=37.0625,-95.677068&spn=177.007106,360&z=1) . Whoa! :)

Andrea Aime added a comment - 20/Mar/08 9:38 AM Bah, not backport on trunk, backport on 2.2.x (uDig is affected by this one as well).

Martin Desruisseaux added a comment - 17/Apr/08 4:14 AM Reopening this issue, since the consequences of allowing the coordinates to go beyong the [-PI ... PI] range are not well understood. For example the Lambert projection performs the following calculation:     sin(lambda * n) where n is some factor. Unfortunatly this is not the same than:     sin((lambda + 2*PI) * n) =     sin(lambda * n + (2*PI) * n) =     sin(lambda * n) * cos(2*PI * n) + cos(lambda * n) * sin(2*PI * n) =     sin(lambda * n) * cos(n) + cos(lambda * n) * sin(n) This is yet more complicated if we want to get the n factor out of sin or cos trigonometric function: http://en.wikipedia.org/wiki/Chebyshev_polynomials So by allowing the longitude or latitude to goes out of their range, we are really getting different result than what we would get by rolling back the longitude (for example) in the -180 ... 180° range. I'm not sure which behavior is the appropriate one...

Martin Desruisseaux added a comment - 17/Apr/08 5:59 AM We should consider deprecating AbstractMathTransform.rollLongitude(double) and remove any usage of it. Generally speaking we should make sure that MapProjection do not introduce any discontinuity (rolling longitude is a discontinuity). Things may be a little bit cleaner if we get the central meridian shift and normalization to unit sphere out of MapProjection code, in an affine transform.

Martin Desruisseaux added a comment - 26/Nov/08 2:12 PM This is cleaned in Geotidy, exactly in the way described in the previous comment. However the "rollLongitude" functionality has not been removed. Instead it has been made configurable as an optional parameter.