Added furlongs test #758
Added furlongs test #758
Conversation
stemann
force-pushed
the
feature/furlongs_test
branch
from
263b478 to
5a38c38
Compare
stemann
force-pushed
the
feature/furlongs_test
branch
from
5a38c38 to
d75ad63
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #758 +/- ##
==========================================
+ Coverage 89.33% 89.79% +0.45%
==========================================
Files 11 11
Lines 1013 1039 +26
==========================================
+ Hits 905 933 +28
+ Misses 108 106 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Seems fine, but is this expected to pass without #665 ? (I would add a big comment linking to Base to explain which code is copied, and which is new tests.) |
|
Not sure, just trying to get the ball rolling :-) @longemen3000 suggested to add this test as a separate PR. Thoughts, @devmotion? |
test/DerivativeTest.jl
Outdated
| Base.exp(f::Furlongs.Furlong) = exp(f.val) | ||
| Base.cos(f::Furlongs.Furlong) = cos(f.val) | ||
| Base.sin(f::Furlongs.Furlong) = sin(f.val) |
There was a problem hiding this comment.
Similar to unitful quantities, shouldn't these only be defined for dimensionless furlongs?
| Base.exp(f::Furlongs.Furlong) = exp(f.val) | |
| Base.cos(f::Furlongs.Furlong) = cos(f.val) | |
| Base.sin(f::Furlongs.Furlong) = sin(f.val) | |
| Base.exp(f::Furlongs.Furlong{0}) = exp(f.val) | |
| Base.cos(f::Furlongs.Furlong{0}) = cos(f.val) | |
| Base.sin(f::Furlongs.Furlong{0}) = sin(f.val) |
There was a problem hiding this comment.
Undid this, at it made the tests fail
Co-authored-by: David Widmann <[email protected]>
test/DerivativeTest.jl
Outdated
| Base.exp(f::Furlongs.Furlong{0}) = exp(f.val) | ||
| Base.cos(f::Furlongs.Furlong{0}) = cos(f.val) | ||
| Base.sin(f::Furlongs.Furlong{0}) = sin(f.val) |
There was a problem hiding this comment.
These should be moved to the Furlongs submodule to avoid type piracy. Alternatively, tests should be based on existing definitions in Furlongs.
|
Bump 🙂 |
|
@devmotion I've added a test that demonstrates the issue outlined in https://github.com/JuliaPhysics/Measurements.jl/pull/178/files/2a41c0cd440def23343d2565ff407eb47916d854#diff-0fafde266294d0a69e0589d46875ecceac83951c1d9a1ae32c084753539ec319R10-R22 - which is fixed by #665 |
4 participants
Added furlongs test - with
Furlongs.jlcopied from test/testhelpers in julia - implementing suggestion described in #665 (comment)