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

intersection line are not complanar with line in plane #319

Closed
volkoshkursk opened this issue Nov 10, 2022 · 13 comments · Fixed by #320
Closed

intersection line are not complanar with line in plane #319

volkoshkursk opened this issue Nov 10, 2022 · 13 comments · Fixed by #320

Comments

@volkoshkursk
Copy link
Contributor

Hello! I generate a plane from points and make a plane from point and normal. Then I make an intersection line with Plane.intersect_plane. When I try to find an intersection point of the intersection line and a line from points, which was used to making a first plane I get an error

    565 if not self.is_coplanar(other, tol=tol):
--> 566     raise ValueError("The lines must be coplanar.")
    568 # Vector from line A to line B.
    569 vector_ab = Vector.from_points(self.point, other.point)

ValueError: The lines must be coplanar.
@ajhynes7
Copy link
Owner

Hello @volkoshkursk,

Could you please post a minimal working example of this? I'll need to see this in code to decide if this is an issue.

@volkoshkursk
Copy link
Contributor Author

volkoshkursk commented Nov 10, 2022

from skspatial.objects import Plane
import numpy as np

points = np.array([[ 80.      , 103.89014 , 378.      ],
                 [ 80.      , 104.      , 377.75623 ],
                 [ 79.957245, 104.      , 378.      ]], dtype=np.float32)

secant = Plane(point=(0,0,0), normal=(1,1,1))
poly = Plane.from_points(points[0],points[1], points[2])

intersection = secant.intersect_plane(poly, abs_tol=1e-6)
line = Line.from_points(points[0], points[1])

line.intersect_line(intersection,  abs_tol=1e-6)

P.S. ubuntu 20, python 3.10
P.P.S. I open an Pull Request which fix this behaviour

@ajhynes7
Copy link
Owner

It doesn't make sense to add a tolerance to the coplanar check in Line.intersect_line. If the lines are not coplanar, then they cannot have an intersection point.

@volkoshkursk
Copy link
Contributor Author

These lines can not be non-coplanar due to the fact that line is build with the points, which was the plane build with and secant is intersection of the plane and another plane. If we check distance between one of the point from points and plane with Plane.distance_point it would not be 0, which would be impossible if we had precise data. The error is happens because we check the coplanarity with more precision that is in data. This situation is equal to is_parallel's limitation of tolerance. My hypothesis also can be proofed graphically.

@ajhynes7
Copy link
Owner

This situation is equal to is_parallel's limitation of tolerance.

It's actually not the same. See this code:

        if self.direction.is_parallel(other.direction, **kwargs):
            raise ValueError("The lines must not be parallel.")

        if not self.is_coplanar(other):
            raise ValueError("The lines must be coplanar.")

Notice that the first check is "if parallel", but the second check is "if NOT coplanar". So passing a tolerance to is_parallel makes that check more strict, but passing a tolerance to is_coplanar would make the check less strict.

If the lines aren't coplanar, then the output of this method will not be an intersection point of the two lines. It'll be the point on line A which is closest to line B, but it won't be an actual intersection point because the lines don't intersect.

I tried your code without the is_coplanar check, and the resulting point is indeed not on line B, which I verified by running this:

>>> point = line.intersect_line(intersection)

>>> intersection.contains_point(point)
False

@volkoshkursk
Copy link
Contributor Author

However, if we calculate the distance between the point and line we get the value, which is less than data precision.

>>> intersection.distance_point(point)
2.9251899693564517e-06

I completely agree, that it would be non-coplanar if the data was enought precise. However, my data's precision is limited by accuracy of sensor. So, difference less then 10^-6 in absolute value is most probably due to inaccuracy of collecting/storage/processing data.

@ajhynes7
Copy link
Owner

Ok, how about I add a keyword argument check_coplanar with default value True? Then you can disable the coplanar check when needed by passing check_coplanar=False.

@volkoshkursk
Copy link
Contributor Author

Sorry for late answer. Yes, it would be solution for my case, because I know that points are coplanar. However, I think, that the problem of non-precise data is popular. Usually, the threshold can be estimated from the description of the task. So, in direct case I think using a tolerance is more expedient.

@ajhynes7
Copy link
Owner

Then I think we should have two types of kwargs in the function signature: is_parallel_kwargs and is_coplanar_kwargs.

@volkoshkursk
Copy link
Contributor Author

volkoshkursk commented Nov 17, 2022

Yes, you are right. I solve this by adding an optional parameter tol to Plane.intersect_line. Also, it was necessary to add this parameter as optional to functions, which depend on Line.intersect_line. More detailed description I add as comment to pull request.

In general, numpy.linalg.optional, witch is called from is_coplanar, has only 2 optional parameters: tol=None and hermitian=False. However, I did't face wis situation when I need to change hermitian=False in context of this task.

Also, we can add an optional parameter dict_of_kwargs={'is_parallel_kwargs': is_parallel_kwargs, 'is_coplanar_kwargs': is_coplanar_kwargs} instead of **kwargs but we lose a backward compatibility then.

@ajhynes7
Copy link
Owner

ajhynes7 commented Nov 19, 2022

Would you be fine with me doing this, or would you rather I review your PR?

@volkoshkursk
Copy link
Contributor Author

I would like you to review my PR

@dancergraham
Copy link

Hello I think you have some trailing whitespaces causing the automatic checks to fail.

trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing src/skspatial/objects/line.py
Fixing src/skspatial/objects/line_segment.py

@ajhynes7 ajhynes7 linked a pull request Dec 29, 2022 that will close this issue
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 a pull request may close this issue.

3 participants