earth_longitude_range() in earth.inc uses invalid values with asin() [#821628]

arcsin is only real for values between -1 & 1.

The earth_longitude_range() function in earth.inc sometimes uses values outside that range.

Comment	File	Size	Author
#5	location-821628-5.patch	2.12 KB	rooby
#5
#1	location-821628-1.patch	654 bytes	rooby
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

File	Size
location-821628-1.patch	654 bytes

Comment #2

rooby CreditAttribution: rooby commented 8 June 2010 at 13:39

Status:

Active

» Needs review

Comment #3

YesCT CreditAttribution: YesCT commented 9 June 2010 at 09:34

+++ earth.inc	2010-06-08 23:30:20.000000000 +1000
@@ -130,7 +130,15 @@ function earth_longitude_range($longitud
-  $diff = asin(sin($angle)/cos($lat));
+  // arcsin is only real for values between -1 & 1.
+  $diff = sin($angle)/cos($lat);
+  if ($diff > 1) {
+    $diff = 1;
+  }
+  else if ($diff < -1) {
+    $diff = -1;
+  }
+  $diff = asin($diff);

code style looks ok. adds comment. approach makes sense to keep values within -1 to 1 (just cuts off values outside that range).

how big is the world? if a search is for locations within say 10,000 miles, would this find "all" locations?
to finish the review, I think what is needed is to
1) get the latest dev
2) apply this patch
3) try searches that are large? and across datelines?
4) reply here with a summary of what functional tests (searches) you tried and the results, and say what the results were before the patch and after the patch

Powered by Dreditor.

Comment #4

rooby CreditAttribution: rooby commented 9 June 2010 at 10:38

Yeah, that was a quick fix without having to think about it. Will have to think it through properly.

The main thing though is that values outside that range cannot be passed into asin because it returns NAN which is not acceptable.

Comment #5

rooby CreditAttribution: rooby commented 13 June 2010 at 07:14

File	Size
location-821628-5.patch	2.12 KB

I have looked at this problem a bit and have come to the following conclusion.

This method is the only real fix for this function. This patch is the same solution as in #1 but a bit neater and with better commenting.

This will mean we don't get NAN results from the asin function.
It also means that for large values of distance the calculation won't be accurate.
This is not regression because getting NAN means the calculation won't work at all, it just means the function has never worked properly for large distance values.

I think this patch should be committed for now but the function needs to be rewritten using a different formula so it works for larger values.

For reference, these are the around about the maximum values of distance that can be used with this function before you start getting inaccurate results, the max value changes relative to the latitude of the point you are measuring from (if my calculations are correct):