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

Is there a typo in hull.py #1

Closed
Phonicavi opened this issue Jul 22, 2020 · 5 comments
Closed

Is there a typo in hull.py #1

Phonicavi opened this issue Jul 22, 2020 · 5 comments
Assignees

Comments

@Phonicavi
Copy link

Sorry for bothering! I was wondering whether there is a typo in the hull.py file, line:190:
in function def distLine(pointA, pointB, pointX)

	vec1 = pointX - pointA
	vec2 = pointX - pointB
	vec3 = pointB - pointA
	vec4 = cross(vec1, vec2)
	if vec2.length() == 0:
		return None

	else:
		return vec4.length()/vec2.length()

You have calculated vec3 but never use it. I think for a line-segment from pointA to pointB, the distance from pointX should be:

	if vec3.length() == 0:
		return None

	else:
		return vec4.length()/vec3.length()

:)

@swapnil96
Copy link
Owner

Looking at this after a long time so I might be rusty in coordinate geometry now

But it seems
What you have written is correct along with that vec4 = cross(vec1, vec3)
I don't know why it didn't come up during testing or not pointed by anyone till now.

Can you create a pull request with necessary changes

  1. Remove vec2
  2. Rename vec3 as vec2, vec4 as vec3
  3. Accordingly the if-else thing

@Phonicavi
Copy link
Author

Phonicavi commented Jul 25, 2020

Thanks for your replying.

Suppose we have a triangle denoted as ABX, with vectors vec1 = \vec{AX}, vec2 = \vec{BX}, vec3 = \vec{AB}, and vec4 = cross(vec1, vec2).
Let \theta be the angle between \vec{XA} and \vec{XB}, the norm of vec4 is actually |AX| * |BX| * sin(\theta).
Consider the area of triangle ABX, suppose the distance from point X to line AB is d, then the area T = 0.5 * |AB| * d, where the base |AB| is the norm of vec3. Again the area of the triangle can also be computed by T = 0.5 * |AX| * |BX| * sin(\theta), see https://en.wikipedia.org/wiki/Law_of_sines#Relationship_to_the_area_of_the_triangle.

Thus d can be computed as |AX| * |BX| * sin(\theta) / |AB|, which is exactly the norm of vec4 / the norm of vec3. Besides if the norm of vec3 is zero (i.e. point A and point B converge into the same position) there is no line segment any more. :)

@swapnil96
Copy link
Owner

Yep, correct again, so there are 2 ways to close this issue

  1. Either we implement the way I mentioned last time, it will take more changes but it will use only 2 vectors and a cross product, not that it matters for time complexity or such, just that it will use the area of a parallelogram formula

  2. We just change the typo of vec2 to vec3 and we are done.

I think I will also go with method 2. If that is all I will close the issue by making that change

@Phonicavi
Copy link
Author

Method 2 looks good to me, thank you!

@swapnil96 swapnil96 self-assigned this Aug 8, 2020
@swapnil96
Copy link
Owner

22e3e3a

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

No branches or pull requests

2 participants