Drop support for Julia < 1.10 #504
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
==========================================
- Coverage 94.11% 93.97% -0.14%
==========================================
Files 14 14
Lines 2905 2904 -1
==========================================
- Hits 2734 2729 -5
- Misses 171 175 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # In future just use `fastabs` from Base.Math | ||
| # https://github.com/JuliaLang/julia/blob/93fb785831dcfcc442f82fab8746f0244c5274ae/base/special/trig.jl#L1057 | ||
| if isdefined(Base.Math, :fastabs) | ||
| import Base.Math: fastabs | ||
| else | ||
| fastabs(x::Number) = abs(x) | ||
| fastabs(x::Complex) = abs(real(x)) + abs(imag(x)) | ||
| end |
There was a problem hiding this comment.
Since fastabs is technically not public, maybe it's safer to keep this?
There was a problem hiding this comment.
Hmm... maybe. Codecov was complaining (correctly) that these lines are not covered by tests anymore. So in that sense technically the fallback is not safe either as it's untested when dropping support for Julia < 1.10.
There was a problem hiding this comment.
Maybe the better approach would be to mark Base.Math.fastabs as public?
There was a problem hiding this comment.
Apart from making Base.Math.fastabs public, the only other "correct" approach would be to only define it in SpecialFunctions, ie to only keep the second branch. Then we would neither rely on internals nor on untested code.
There was a problem hiding this comment.
Yeah, maybe that's not too bad, it's two very simple methods, and unlikely to need maintenance.
There was a problem hiding this comment.
OK, I removed the first branch 🙂
2 participants
See #503 (comment)