Add warning for all possible paths and cyclic path #20131

sidhu1012 · 2020-09-22T12:39:33Z

References to other Issues or PRs

fix #20129

Brief description of what is fixed or changed

Added User Warnings for all possible paths and userwarnings for cyclic paths.

Other comments

Release Notes

physics.vector
- Added user warnings for all possible paths are found in particle.py.
- Added user warning for cyclic paths in particle.py.

sympy-bot · 2020-09-22T12:39:35Z

✅

Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

physics.vector
- Added user warnings for all possible paths are found in particle.py. (#20131 by @sidhu1012)
- Added user warning for cyclic paths in particle.py. (#20131 by @sidhu1012)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. --> 
fix #20129 

#### Brief description of what is fixed or changed
Added User Warnings for all possible paths and userwarnings for cyclic paths.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->

- physics.vector
      - Added user warnings for all possible paths are found in particle.py.
      - Added user warning for cyclic paths in particle.py.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

codecov · 2020-09-22T18:19:21Z

Codecov Report

Merging #20131 into master will increase coverage by 11.401%.
The diff coverage is 88.854%.

@@              Coverage Diff               @@
##            master    #20131        +/-   ##
==============================================
+ Coverage   64.466%   75.867%   +11.401%     
==============================================
  Files          671       671                
  Lines       173707    173809       +102     
  Branches     41020     41053        +33     
==============================================
+ Hits        111982    131864     +19882     
+ Misses       55307     36203     -19104     
+ Partials      6418      5742       -676

sidhu1012 · 2020-09-23T09:48:53Z

@moorepants Pls check now

oscarbenjamin · 2020-09-23T09:57:25Z

Maybe this code should use the topological_sort function from iterables:
https://docs.sympy.org/latest/modules/utilities/iterables.html#sympy.utilities.iterables.topological_sort

Or otherwise maybe a more generic routine could be added to iterables and used here.

sidhu1012 · 2020-09-23T10:39:52Z

Or otherwise maybe a more generic routine could be added to iterables and used here.

Can you please elaborate

asmeurer · 2020-09-24T01:27:58Z

The release notes don't need to mention test cases. They should only be for end-users.

asmeurer · 2020-09-24T01:29:51Z

I would like for @moorepants to confirm this is what he had in mind. My opinion is we generally should be minimal in omitting warnings in SymPy, though there are legitimate use-cases for them.

moorepants · 2020-09-24T06:19:53Z

These warnings will only occur if the user sets up their kinematics incorrectly. I think it is a useful feature for the user and it is also pertinent now that this automatic calculation of velocities has been implemented. We should, in general, let the user know if they incorrectly define relationships among points or reference frames.

sympy/physics/vector/tests/test_point.py

sympy/physics/vector/point.py

sidhu1012 · 2020-09-25T07:56:59Z

@moorepants Check now please

asmeurer · 2020-09-25T08:19:50Z

Should it rather be an error?

Another issue with warnings is that they are only emitted once by default (though that might not be the case if the message is different).

moorepants · 2020-09-25T08:34:27Z

No because the selection in the algorithm could be correct and the problem could successfully be defined if the extraneous position connection doesn't effect the result.

sidhu1012 · 2020-09-25T08:36:37Z

Should it rather be an error?

These warnings shouldn't be error , as the calculated velocity is correct , but the veloctiy that is calculated would be dependent on how user creates the point tree. So I think the idea is to tell user that calculated veloctiy is dependent on how they define relation between points.

sidhu1012 · 2020-09-25T12:17:03Z

@moorepants Does it seems right or require further changes?

sidhu1012 · 2020-09-28T12:55:59Z

@moorepants Does it seems right or require further changes?

@moorepants Please review it

moorepants · 2020-09-28T13:49:23Z

Please be patient, as we are all doing this as volunteers. I'll review when I have the time. I recommend working on other PRs while you wait for reviews.

sympy/physics/vector/point.py

moorepants · 2020-09-30T07:47:10Z

sympy/physics/vector/tests/test_point.py

+    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


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?

multiplath warning

Added comment

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.

Added comments and also ensured that correct warning is being raised

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.

moorepants · 2020-09-30T13:08:20Z

This LGTM and can be merged when tests pass.

sidhu1012 · 2020-10-01T05:45:49Z

This LGTM and can be merged when tests pass.

Thanks 😄

sidhu1012 closed this Sep 22, 2020

sidhu1012 reopened this Sep 22, 2020

warning for cyclic paths and other possible paths along with test cases

d4119a9

sidhu1012 changed the title ~~Add warning for multiple shortest paths~~ Add warning for all possible paths and cyclic path Sep 23, 2020

Smit-create added the physics.vector label Sep 23, 2020

asmeurer requested a review from moorepants September 24, 2020 01:28

moorepants reviewed Sep 24, 2020

View reviewed changes

sympy/physics/vector/tests/test_point.py Show resolved Hide resolved

moorepants reviewed Sep 24, 2020

View reviewed changes

sympy/physics/vector/point.py Outdated Show resolved Hide resolved

moorepants reviewed Sep 24, 2020

View reviewed changes

sympy/physics/vector/point.py Outdated Show resolved Hide resolved

sidhu1012 closed this Sep 24, 2020

sidhu1012 reopened this Sep 24, 2020

sidhu1012 closed this Sep 24, 2020

sidhu1012 reopened this Sep 24, 2020

Updated warning message, added new test cases for checking msg

5d3597b

sidhu1012 requested a review from moorepants September 27, 2020 10:44

moorepants reviewed Sep 30, 2020

View reviewed changes

sympy/physics/vector/point.py Outdated Show resolved Hide resolved

moorepants reviewed Sep 30, 2020

View reviewed changes

sympy/physics/vector/point.py Outdated Show resolved Hide resolved

moorepants reviewed Sep 30, 2020

View reviewed changes

sympy/physics/vector/point.py Outdated Show resolved Hide resolved

moorepants reviewed Sep 30, 2020

View reviewed changes

sidhu1012 added 2 commits September 30, 2020 16:07

Updated warning msg as per suggestion

01b91dc

Added comments for warnings raised

8a61a15

moorepants merged commit adaf9ab into sympy:master Oct 1, 2020

sidhu1012 deleted the vel_warning branch October 1, 2020 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for all possible paths and cyclic path #20131

Add warning for all possible paths and cyclic path #20131

sidhu1012 commented Sep 22, 2020 •

edited

sympy-bot commented Sep 22, 2020 •

edited

codecov bot commented Sep 22, 2020 •

edited

sidhu1012 commented Sep 23, 2020

oscarbenjamin commented Sep 23, 2020

sidhu1012 commented Sep 23, 2020

asmeurer commented Sep 24, 2020

asmeurer commented Sep 24, 2020

moorepants commented Sep 24, 2020

sidhu1012 commented Sep 25, 2020

asmeurer commented Sep 25, 2020

moorepants commented Sep 25, 2020

sidhu1012 commented Sep 25, 2020

sidhu1012 commented Sep 25, 2020

sidhu1012 commented Sep 28, 2020

moorepants commented Sep 28, 2020

moorepants Sep 30, 2020

sidhu1012 Sep 30, 2020

sidhu1012 Sep 30, 2020

moorepants Sep 30, 2020

sidhu1012 Sep 30, 2020

moorepants Sep 30, 2020

moorepants commented Sep 30, 2020

sidhu1012 commented Oct 1, 2020

Add warning for all possible paths and cyclic path #20131

Add warning for all possible paths and cyclic path #20131

Conversation

sidhu1012 commented Sep 22, 2020 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Sep 22, 2020 • edited

codecov bot commented Sep 22, 2020 • edited

Codecov Report

sidhu1012 commented Sep 23, 2020

oscarbenjamin commented Sep 23, 2020

sidhu1012 commented Sep 23, 2020

asmeurer commented Sep 24, 2020

asmeurer commented Sep 24, 2020

moorepants commented Sep 24, 2020

sidhu1012 commented Sep 25, 2020

asmeurer commented Sep 25, 2020

moorepants commented Sep 25, 2020

sidhu1012 commented Sep 25, 2020

sidhu1012 commented Sep 25, 2020

sidhu1012 commented Sep 28, 2020

moorepants commented Sep 28, 2020

moorepants Sep 30, 2020

Choose a reason for hiding this comment

sidhu1012 Sep 30, 2020

Choose a reason for hiding this comment

sidhu1012 Sep 30, 2020

Choose a reason for hiding this comment

moorepants Sep 30, 2020

Choose a reason for hiding this comment

sidhu1012 Sep 30, 2020

Choose a reason for hiding this comment

moorepants Sep 30, 2020

Choose a reason for hiding this comment

moorepants commented Sep 30, 2020

sidhu1012 commented Oct 1, 2020

sidhu1012 commented Sep 22, 2020 •

edited

sympy-bot commented Sep 22, 2020 •

edited

codecov bot commented Sep 22, 2020 •

edited