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

bugfix: handle problems acos arg > 1 #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tjansson60
Copy link
Contributor

While processing the shoreline set GSHHS_c_L1 with 742 shapes a
problem was encountered where cos_value = 1.00000000000048 which meant
that the acos raised an ValueError: math domain error and the graph
could not be built.

This patch checks if the arguments are very close to -1 or 1 and returns
the expected values. If the values are not close to either of these
values the ValueError is still raised.

Examples of values in angle2 when problem was first encountered:
point_a = (112.96, -25.49)
point_b = (45.00, -25.49)
point_c = (45.00, -25.49)
a = 6.938890000001394e-07
b = 4618.002849783456
c = 4618.11606498842
cos_value = 1.00000000000048
T = 10000000000000
T2 = 10000000000000.0

While processing the shoreline set GSHHS_c_L1 with 742 shapes a
problem was encountered where cos_value = 1.00000000000048 which meant
that the acos raised an ValueError: math domain error and the graph
could not be built.

This patch checks if the arguments are very close to -1 or 1 and returns
the expected values. If the values are not close to either of these
values the ValueError is still raised.

point_a   = (112.96, -25.49)
point_b   = (45.00, -25.49)
point_c   = (45.00, -25.49)
a         = 6.938890000001394e-07
b         = 4618.002849783456
c         = 4618.11606498842
cos_value = 1.00000000000048
T         = 10000000000000
T2        = 10000000000000.0
@TaipanRex
Copy link
Owner

I'll need to performance test this.

Check out the discussion on angle2() in #28, by setting COLIN_TOLERANCE to a lower value, like 8, it should solve this problem without any performance hit. Its not explained well in the code and I'm working on better naming the COLIN_TOLERANCE, T1, T2 variables and also allowing the user to set COLIN_TOLERANCE through the API (see #29).

@tjansson60
Copy link
Contributor Author

Ahh, had not seen #28, but yes it the same. I think it should not hit performance with a try except loop as it only executes the except if the try fails, so I am assuming it should be be somewhat the same. I guess this ties together with your work on a performance testing framework in #30.

TaipanRex added a commit that referenced this pull request May 30, 2018
Still seeing some errors so reduced the COLIN_TOLERANCE from 13 to 10.
This solves the specific example given in #33 and I also mentioned
in #28 that I would reduce the tolerance to better avoid these types
of errors.

@tjansson60 has a suggested solution that does not involve truncating
the cos_value, I need to performance test this first.

See: #33, #28, #19
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.

2 participants