Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve the floating-point precision of quantities in uconvert #754

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Preserve the floating-point precision of quantities in uconvert #754

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13aae79
Avoid converting the float type of quantities
eliascarv Dec 5, 2024
d1605dc
Fix typo
eliascarv Dec 5, 2024
9059620
Apply suggestions
eliascarv Dec 10, 2024
a20b45a
Apply suggestions
eliascarv Dec 11, 2024
e156e51
Fix Ambiguity tests
eliascarv Dec 11, 2024
d4e1e42
Apply suggestions
eliascarv Dec 11, 2024
de56f8c
Update src/conversion.jl
eliascarv Dec 11, 2024
dc04b9b
Update UnitConversionFactor definition
eliascarv Dec 11, 2024
bd81b57
Update comments
eliascarv Dec 11, 2024
8becfd6
Update docstring
eliascarv Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions src/conversion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
"""
struct UnitConversionFactor{x} <: AbstractIrrational end
Conversion factor with value `x`.

Used by the [`convfact`](@ref) function in floating point conversion factors
to preserve the precision of quantities.
"""
struct UnitConversionFactor{x} <: AbstractIrrational end
eliascarv marked this conversation as resolved.
Show resolved Hide resolved

Base.:(==)(::UnitConversionFactor{a}, ::UnitConversionFactor{b}) where {a, b} = a == b
Base.hash(x::UnitConversionFactor, h::UInt) = 3 * objectid(x) - h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Base.hash(x::UnitConversionFactor, h::UInt) = 3 * objectid(x) - h
Base.hash(::UnitConversionFactor{x}, h::UInt) where {x} = hash(x, h)

Since UnitConversionFactor is (for now) always exactly equal to a Float64, it should hash the same. We should also implement ==(::UnitConversionFactor, ::Real) and ==(::Real, ::UnitConversionFactor). However, since this type is only used internally, it probably doesn’t matter too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sostock adjusted. Do you think the == with Real is necessary? If so, I can add the methods and tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably isn’t necessary, since == or hash is likely never used on this type. But I still think hash and isequal (which falls back to ==) should be consistent. So, I would either change back hash to the old implementation (where the hashes are never equal to those of a Float64) or add the == methods.

I think we could actually remove the hash and == methods and they would still be consistent using the fallback methods.

Base.BigFloat(::UnitConversionFactor{x}) where {x} = BigFloat(x)
Base.Float64(::UnitConversionFactor{x}) where {x} = Float64(x)
Base.Float32(::UnitConversionFactor{x}) where {x} = Float32(x)

"""
convfact(s::Units, t::Units)
Find the conversion factor from unit `t` to unit `s`, e.g., `convfact(m, cm) == 1//100`.
Expand Down Expand Up @@ -30,13 +45,14 @@ Find the conversion factor from unit `t` to unit `s`, e.g., `convfact(m, cm) ==
ex = numerator(ex)
end

result = (inex ≈ 1.0 ? 1 : inex) * ex
if fp_overflow_underflow(inex_orig, result)
ex = (inex ≈ 1.0 ? 1 : inex) * ex
if fp_overflow_underflow(inex_orig, ex)
eliascarv marked this conversation as resolved.
Show resolved Hide resolved
throw(ArgumentError(
"Floating point overflow/underflow, probably due to large " *
"exponents and/or SI prefixes in units"
))
end
result = ex isa AbstractFloat ? UnitConversionFactor{ex}() : ex
return :($result)
end

Expand Down
6 changes: 6 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,12 @@ end
# Issue 647:
@test uconvert(u"kb^1000", 1u"kb^1001 * b^-1") === 1000u"kb^1000"
@test uconvert(u"kOe^1000", 1u"kOe^1001 * Oe^-1") === 1000u"kOe^1000"
# Issue 753:
# avoid converting the float type of quantities
@test Unitful.numtype(uconvert(m, 100f0cm)) === Float32
eliascarv marked this conversation as resolved.
Show resolved Hide resolved
@test Unitful.numtype(uconvert(cm, (1f0π + im) * m)) === ComplexF32
@test Unitful.numtype(uconvert(rad, 360f0°)) === Float32
@test Unitful.numtype(uconvert(°, (2f0π + im) * rad)) === ComplexF32
# Floating point overflow/underflow in uconvert can happen if the
# conversion factor is large, because uconvert does not cancel
# common basefactors (or just for really large exponents and/or
Expand Down
Loading