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

Add warning for all possible paths and cyclic path #20131

Merged
merged 4 commits into from Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 20 additions & 2 deletions sympy/physics/vector/point.py
@@ -1,6 +1,7 @@
from __future__ import print_function, division
from .vector import Vector, _check_vector
from .frame import _check_frame
from warnings import warn

__all__ = ['Point']

Expand Down Expand Up @@ -529,24 +530,41 @@ def vel(self, frame):

_check_frame(frame)
if not (frame in self._vel_dict):
valid_neighbor_found = False
is_cyclic = False
visited = []
queue = [self]
candidate_neighbor = []
while queue: #BFS to find nearest point
node = queue.pop(0)
if node not in visited:
visited.append(node)
for neighbor, neighbor_pos in node._pos_dict.items():
if neighbor in visited:
continue
try:
neighbor_pos.express(frame) #Checks if pos vector is valid
except ValueError:
continue
if neighbor in queue:
is_cyclic = True
try :
neighbor_velocity = neighbor._vel_dict[frame] #Checks if point has its vel defined in req frame
except KeyError:
queue.append(neighbor)
continue
self.set_vel(frame, self.pos_from(neighbor).dt(frame) + neighbor_velocity)
return self._vel_dict[frame]
candidate_neighbor.append(neighbor)
if not valid_neighbor_found:
self.set_vel(frame, self.pos_from(neighbor).dt(frame) + neighbor_velocity)
valid_neighbor_found = True
if is_cyclic:
warn('Multiple points have their position defined with respect to one point.')
sidhu1012 marked this conversation as resolved.
Show resolved Hide resolved
if len(candidate_neighbor) > 1:
warn('Velocity automatically calculated based on point ' +
candidate_neighbor[0].name + ' but it is also possible from ' +
sidhu1012 marked this conversation as resolved.
Show resolved Hide resolved
str(candidate_neighbor[1:]) + '. Velocities from these points are not the same.')
sidhu1012 marked this conversation as resolved.
Show resolved Hide resolved
if valid_neighbor_found:
return self._vel_dict[frame]
else:
raise ValueError('Velocity of point ' + self.name + ' has not been'
' defined in ReferenceFrame ' + frame.name)
Expand Down
74 changes: 71 additions & 3 deletions sympy/physics/vector/tests/test_point.py
@@ -1,6 +1,6 @@
from sympy.physics.vector import dynamicsymbols, Point, ReferenceFrame
from sympy.testing.pytest import raises

from sympy.testing.pytest import raises, ignore_warnings
import warnings

def test_point_v1pt_theorys():
q, q2 = dynamicsymbols('q q2')
Expand Down Expand Up @@ -216,7 +216,10 @@ def test_auto_point_vel_shortest_path():
O1 = Point('O1')
O1.set_pos(O, q2 * B.z)
P4.set_pos(O1, q1 * B.x + q2 * B.z)
assert P4.vel(B) == q1.diff(t) * B.x + u2 * B.y + 2 * q2.diff(t) * B.z
with warnings.catch_warnings():
warnings.simplefilter('error')
with ignore_warnings(UserWarning):
assert P4.vel(B) == q1.diff(t) * B.x + u2 * B.y + 2 * q2.diff(t) * B.z
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be helpful to have a comment explaining what is happening in each warning catch, as it isn't obvious from a quick read of the code. Does this one now cause a warning? Which one the cyclic or multipath one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiplath warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here it is supposed to get that warning? It isn't clear what each warning catch (in each test is doing). You should write a comment just above with in each one that is something like "there are two possible paths in this point tree, thus a warning is raised". Should you also ensure that the correct warning is raised, as there are two possible warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments and also ensured that correct warning is being raised

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about requesting that, I missed that the names of the functions explained what was going on too. I thought they were all old tests you added warning catching too. Regardless, it is clearer now. Thanks.


def test_auto_point_vel_connected_frames():
t = dynamicsymbols._t
Expand All @@ -230,3 +233,68 @@ def test_auto_point_vel_connected_frames():
raises(ValueError, lambda: P.vel(N))
N.orient(B, 'Axis', (q, B.x))
assert P.vel(N) == (u + q1.diff(t)) * N.x + q2.diff(t) * B.y - q2 * q.diff(t) * B.z

def test_auto_point_vel_multiple_paths_warning_arises():
q, u = dynamicsymbols('q u')
N = ReferenceFrame('N')
O = Point('O')
P = Point('P')
Q = Point('Q')
R = Point('R')
P.set_vel(N, u * N.x)
Q.set_vel(N, u *N.y)
R.set_vel(N, u * N.z)
O.set_pos(P, q * N.z)
O.set_pos(Q, q * N.y)
O.set_pos(R, q * N.x)
with warnings.catch_warnings():
warnings.simplefilter("error")
raises(UserWarning ,lambda: O.vel(N))

def test_auto_vel_cyclic_warning_arises():
P = Point('P')
P1 = Point('P1')
P2 = Point('P2')
P3 = Point('P3')
N = ReferenceFrame('N')
P.set_vel(N, N.x)
P1.set_pos(P, N.x)
P2.set_pos(P1, N.y)
P3.set_pos(P2, N.z)
P1.set_pos(P3, N.x + N.y)
with warnings.catch_warnings():
warnings.simplefilter("error")
raises(UserWarning ,lambda: P2.vel(N))
sidhu1012 marked this conversation as resolved.
Show resolved Hide resolved

def test_auto_vel_cyclic_warning_msg():
P = Point('P')
P1 = Point('P1')
P2 = Point('P2')
P3 = Point('P3')
N = ReferenceFrame('N')
P.set_vel(N, N.x)
P1.set_pos(P, N.x)
P2.set_pos(P1, N.y)
P3.set_pos(P2, N.z)
P1.set_pos(P3, N.x + N.y)
with warnings.catch_warnings(record = True) as w:
warnings.simplefilter("always")
P2.vel(N)
assert issubclass(w[-1].category, UserWarning)
assert 'Multiple points have their position defined with respect to one point.' in str(w[-1].message)

def test_auto_vel_multiple_path_warning_msg():
N = ReferenceFrame('N')
O = Point('O')
P = Point('P')
Q = Point('Q')
P.set_vel(N, N.x)
Q.set_vel(N, N.y)
O.set_pos(P, N.z)
O.set_pos(Q, N.y)
with warnings.catch_warnings(record = True) as w:
warnings.simplefilter("always")
O.vel(N)
assert issubclass(w[-1].category, UserWarning)
assert 'Velocity automatically calculated based on point' in str(w[-1].message)
assert 'Velocities from these points are not the same.' in str(w[-1].message)