Bounds on acos usages; fix a couple of links in readme#108
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
==========================================
+ Coverage 76.47% 77.04% +0.56%
==========================================
Files 24 24
Lines 5136 5136
==========================================
+ Hits 3928 3957 +29
+ Misses 1208 1179 -29 ☔ View full report in Codecov by Sentry. |
2ecb3ed to
1b27a7b
Compare
more tests more
5b2207a to
62a98a9
Compare
|
I have a problem with the fix for |
There was a problem hiding this comment.
I have a problem with the fix for log since s can be negative, so the s/|q| should be bounded in the range [-1, 1]. I think the other fixes are ok.
There was a problem hiding this comment.
thanks for pointing this out! change pushed in 2638584
petercorke
left a comment
There was a problem hiding this comment.
useful fixes in general.
some issue with log of Quaternion, good to have other opinions on this, but I think the bounds should be [-1, 1]. Not sure whether it's faster to use min and max or numpy's clip
| q1 = Quaternion([4, 3, 2, 1]) | ||
| q2 = Quaternion([-1, 2, -3, 4]) | ||
|
|
||
| self.assertTrue(isscalar(q1.log().s)) |
There was a problem hiding this comment.
Do we need to test isscalar and isvector twice?
There was a problem hiding this comment.
right! repetitive tests removed, in 811ac8d
There was a problem hiding this comment.
could you please take another look? @myeatman-bdai (i can't seem to re-request review from the UI...)
Change implemented as requested - I am setting this to a review comment to avoid bothering Peter too much. I will take on any followups or TODO's as necessary.
This is a first pass for the task "Bounds on acos and other functions with bounded domains", with focus on
acosandasinusages.math.acos,math.asinreviewed;