`calcz` doesn't handle many numeric types due to overly-restrictive type-signature in `cardan`.
wnoise opened this issue · 1 comments
wnoise commented
Example 1: bigfloats
spechum(MoistAir, big"300", WetBulb, 291.15, 101325.0)
gives
ERROR: MethodError: no method matching cardan(::Tuple{BigFloat, BigFloat, Float64, Float64})
Closest candidates are:
cardan(::NTuple{4, T}) where T<:AbstractFloat at ~/external/Psychro.jl/src/utilities.jl:80
ForwardDiff also fails:
using ForwardDiff
spechum_t(t) = spechum(MoistAir, t, WetBulb, 291.0, 101325.0)
ForwardDiff.derivative(spechum_t, 293.0)
with
ERROR: MethodError: no method matching cardan(::Tuple{ForwardDiff.Dual{ForwardDiff.Tag{typeof(spechum_t), Float64}, Float64, 1}, ForwardDiff.Dual{ForwardDiff.Tag{typeof(spechum_t), Float64}, Float64, 1}, Float64, Float64})
Closest candidates are:
cardan(::NTuple{4, T}) where T<:AbstractFloat at ~/external/Psychro.jl/src/utilities.jl:80
The obvious thing to do is just remove the constraint (and the uses of T()
inside the function).
This works fine (including still working with Unitful quantities, as those are dealt with higher up in the call-stack).
However examining the function closer, cardan
is passed information it cannot use, as the last element must always be 1.
In addition, as cardan
is not exported, and only called calcz
, the second-to-last element is always -1.0, but isn't reflected in cardan
.
I would recommend:
- rename
cardan
to make clear it isn't the normal cardano algorithm and only handles cases with 1.0x^3 + ..., - specialize it to 1.0 x^3 - 1.0 x^2 + ...,
- make it take two arguments, rather than a tuple, with each argument separately descending from Real.
- actually work out the consequences, though julia/llvm might be smart enough to just deal with it setting E1
- keep the constants as Float64 (as almost anything will be able to multiply by Float64s, though 1/3 as 0.33333... loses some precision with e.g. BigFloat. 1//3 might specialize well enough though?)
I'd be glad to submit a pull-request that does whatever subset of this you think is appropriate.
wnoise commented
Whoops. Wrong repo, apologies.