Conversation
stemann
force-pushed
the
feature/furlongs_test
branch
2 times, most recently
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 90.73% 90.73%
=======================================
Files 11 11
Lines 1069 1069
=======================================
Hits 970 970
Misses 99 99 ☔ 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, as it made the tests fail
|
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 |
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
…sionless Furlongs
stemann
force-pushed
the
feature/furlongs_test
branch
from
d29dacf to
af8176e
Compare
|
|
||
| f(x) = exp(x) + 4*sin(x)*oneunit(x) | ||
|
|
||
| @test ForwardDiff.derivative(f, furlong) == exp(furlong) + 4*cos(furlong) |
There was a problem hiding this comment.
Would any of gradient etc fail for Furlongs? If construct_seeds errors, it should also be possible to provoke errors in user-facing functions, and testing the latter seems much more relevant and future-proof than testing an internal function.
| 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.
These still don't seem reasonable to me - IMO they only exist for dimensionless numbers (ie Furlong{0}). This is also how they're defined in Unitful IIRC.
There was a problem hiding this comment.
Cf. #758 (comment)
The test currently entails exp etc. being defined for ::Furlongs.Furlong{2, Float64}`:
https://github.com/JuliaDiff/ForwardDiff.jl/actions/runs/17889140228/job/50867233017#step:6:156
If my comment makes no sense, please elaborate on what is unreasonable - I'm quite sure I may not have understood what you mean.
There was a problem hiding this comment.
My point is that I don't think it makes sense to define exp, sin and cos for numbers with dimensions. For instance, it's clear what 3m^2 + 2m^2 should be, or similarly 3 * 2m^2 or 3m^2 * 2m^2; but it's not clear what exp(3m^2) should be - if you'd plug 3m^2 into the Taylor series expansion of exp, you'd get a sum of terms with different (and ever increasing number of) dimensions.
Of course, I see that removing these definitions will break the tests. But IMO the conclusion should be to use other functions in the test, not to keep these definitions.
4 participants
Added furlongs test - with
Furlongs.jlcopied from test/testhelpers in julia - implementing suggestion described in #665 (comment)