Skip to content

Update more tolerance usages, add a few docstring and unit test#106

Merged
jcao-bdai merged 1 commit intomasterfrom
SW-591/add-test-coverage
Nov 30, 2023
Merged

Update more tolerance usages, add a few docstring and unit test#106
jcao-bdai merged 1 commit intomasterfrom
SW-591/add-test-coverage

Conversation

@jcao-bdai
Copy link
Copy Markdown
Contributor

@jcao-bdai jcao-bdai commented Nov 29, 2023

  • another pass by searching and checking all _eps usages (previously missed when searching with tol keyword);
  • fix a few docstring issues;
  • fix a minor bug in angle_wrap;
  • improve test coverage for math methods (passing up visualization methods for now).

@jcao-bdai jcao-bdai marked this pull request as ready for review November 29, 2023 22:23
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 29, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fd9e6a0) 75.62% compared to head (6939ed8) 76.49%.

❗ Current head 6939ed8 differs from pull request most recent head a81c21c. Consider uploading reports for the commit a81c21c to get more accurate results

Files Patch % Lines
spatialmath/quaternion.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   75.62%   76.49%   +0.86%     
==========================================
  Files          24       24              
  Lines        5132     5134       +2     
==========================================
+ Hits         3881     3927      +46     
+ Misses       1251     1207      -44     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcao-bdai jcao-bdai force-pushed the SW-591/add-test-coverage branch from c1612af to 6939ed8 Compare November 30, 2023 15:53
@jcao-bdai jcao-bdai changed the title Add test coverage Update more tolerance usages, add a few docstring and unit test Nov 30, 2023
@jcao-bdai jcao-bdai force-pushed the SW-591/add-test-coverage branch from 6939ed8 to a81c21c Compare November 30, 2023 16:00
Copy link
Copy Markdown
Collaborator

@myeatman-bdai myeatman-bdai left a comment

Choose a reason for hiding this comment

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

Looks good to me.

T = np.zeros((n, n), dtype="O")
else:
# numeric matrix
if not isinstance(R, np.ndarray):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this removed? Is it redundant, or just not corrrect?

Copy link
Copy Markdown
Contributor Author

@jcao-bdai jcao-bdai Nov 30, 2023

Choose a reason for hiding this comment

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

sorry for the confusion - i squashed a commit probably when you were in the middle of reviewing. i actually restored this line to be extra cautious.

i made the initial change because there seemed to be a check, several lines above, of the same intention, so i thought this line might be redundant.

Comment thread spatialmath/base/vectors.py Outdated

if iszerovec(S, tol=tol):
raise ValueError("zero norm")
return (None, None)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this more consistent with the rest of the code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sorry for the confusion - i squashed a commit probably when you were in the middle of reviewing.

here i added an inline comment: # according to "note" in docstring.

elif mode == "0:pi":
return wrap_0_pi(theta)
elif mode == "0:pi":
elif mode == "-pi/2:pi/2":
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nice catch

@jcao-bdai jcao-bdai merged commit 3dec5a8 into master Nov 30, 2023
@jcao-bdai jcao-bdai deleted the SW-591/add-test-coverage branch November 30, 2023 19:45
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