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.

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

Compatibility with zero(::Type{T}) #76

Open
MilesCranmer opened this issue Oct 27, 2023 · 6 comments
Open

Compatibility with zero(::Type{T}) #76

MilesCranmer opened this issue Oct 27, 2023 · 6 comments

Comments

@MilesCranmer
Copy link
Member

MilesCranmer commented Oct 27, 2023

It seems like a reason DynamicQuantities.jl is incompatible with many libraries right now is due to their usage of zero(::Type{T}), which works for Unitful but not for DynamicQuantities.jl.

Therefore I wonder if there is a way of defining something like:

zero(::Type{Q}) where {Q<:AbstractQuantity} = AutoQuantityZero{Q}()

which could avoid checking for any dimension errors in operations, and act as a neutral zero.

The problem with this is type instability, but perhaps that's better than simply throwing an error...? Then as packages adapt they would get type stability again. (And of course any package already using zero(::T) would maintain type stability).

Thoughts @gaurav-arya?

We could even think of adding a special boolean field to Quantity to indicate this... thus making everything type stable. But it's a bit worrisome...

@gaurav-arya
Copy link
Collaborator

gaurav-arya commented Oct 27, 2023

It doesn't necessarily have to be type unstable. For example, you could have a boolean valid bit in a Dimensions object, with valid = 0 in this case. I did something like this in StochasticAD.jl: https://github.com/gaurav-arya/StochasticAD.jl/blob/e13ee20616ec8cc053dab181ef9bccd160b0d12a/src/backends/pruned_aggressive.jl#L50-L53

In theory, the type approach + union splitting could also work. This could in theory be superior to the valid bit approach in that the branch will only be created in the case where one of the units could feasibly be invalid, as opposed to all the time for every addition of quantities. So this seems promising -- I just don't personally have a good mental model for when union splitting runs, its performance, etc.

Am I understanding correctly that we'd want to make the units of this zero a "wildcard", i.e willing to assume the units of any other type? It would make code such as zero(typeof(1u"m")) + 1u"s" work, but perhaps that's OK / a worthwhile tradeoff. [Edit: re-reading, yes I am pretty sure this is what you mean:)]

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Jan 12, 2024

Pasted from #77

I think one alternative is to allow a user to temporarily disable dimensional analysis in a particular scope, so that the first call of a Base.:+(::Quantity, ::Number)would simply take the first dimensions it sees. Maybe like

using DynamicQuantities

@dimension_globbing begin
    u"km/s" + one(typeof(u"km/s"))
end

which would result as 1001 m/s. The one(u"km/s") == 1.0 but since you turned on dimension globbing, the 1.0 would automatically take the units m/s from the u"km/s" part.

@mikeingold
Copy link
Contributor

mikeingold commented Jan 17, 2024

Just came over to mention this occurs when trying to compose DynamicQuantities.jl with the geometric representations from Meshes.jl.

using DynamicQuantities
using Meshes

Meshes.Point(1.0m, 2.0m)

Example error stack trace:

ERROR: Cannot create an additive identity for a `UnionAbstractQuantity` type, as the dimensions are unknown. Please use `zero(::UnionAbstractQuantity)` instead.
Stacktrace:
 [1] error(s::String)
   @ Base .\error.jl:35
 [2] zero(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ DynamicQuantities C:\Users\x\.julia\packages\DynamicQuantities\HYcKp\src\utils.jl:280
 [3] float(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
   @ Base .\float.jl:311
 [4] Vec(coords::Tuple{DynamicQuantities.Quantity{…}, DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:46
 [5] Vec(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\vectors.jl:58
 [6] Point(::DynamicQuantities.Quantity{Float64, Dimensions{…}}, ::Vararg{DynamicQuantities.Quantity{…}})
   @ Meshes C:\Users\x\.julia\packages\Meshes\Lb9po\src\primitives\point.jl:48
 [7] top-level scope
   @ REPL[15]:1
Some type information was truncated. Use `show(err)` to see complete types.

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Jan 17, 2024

Thanks, I posted a comment there as well.

I'd be open to the idea #76 (comment) if someone wants to try it. It's not really a robust approach to this, but it is a temporary workaround in case there are issues like this in a downstream package.

@mikeingold
Copy link
Contributor

Thanks again for the quick fix to float(T), @MilesCranmer !

I just updated my LineIntegrals.jl package to enable tests with DynamicQuantities.jl and have run into a new hurdle. I don't time to dig in more at the moment, but it looks like Julia's isapprox also runs into issues with zero(T) and something about Quantity<:Real, e.g.:

Scalar-Valued Functions: Error During Test at C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:159
  Test threw exception
  Expression: isapprox(integral(f, unit_circle), (2π) * Ω; atol = 0.15Ω)
  TypeError: in keyword argument atol, expected Real, got a value of type DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}
  Stacktrace:
   [1] isapprox(l::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}, r::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}; kws::@Kwargs{atol::DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
     @ DynamicQuantities C:\Users\mikei\.julia\packages\DynamicQuantities\jkfTz\src\utils.jl:227
   [2] eval_test(evaluated::Expr, quoted::Expr, source::LineNumberNode, negate::Bool)
     @ Test C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:367
   [3] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:669 [inlined]
   [4] macro expansion
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:159 [inlined]
   [5] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]
   [6] macro expansion
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:154 [inlined]
   [7] macro expansion
     @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]
   [8] top-level scope
     @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:128

Vector-Valued Functions: Error During Test at C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:164
  Test threw exception
  Expression: integral(f, seg_ne)  [sqrt(2), sqrt(2), sqrt(2)] .* Ω
  Cannot create an additive identity for a `UnionAbstractQuantity` type, as the dimensions are unknown. Please use `zero(::UnionAbstractQuantity)` instead.
  Stacktrace:
    [1] error(s::String)
      @ Base .\error.jl:35
    [2] zero(::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
      @ DynamicQuantities C:\Users\mikei\.julia\packages\DynamicQuantities\jkfTz\src\utils.jl:280
    [3] real(T::Type)
      @ Base .\complex.jl:120
    [4] rtoldefault(x::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, y::Type{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, atol::Int64)
      @ Base .\floatfuncs.jl:347
    [5] isapprox(x::Vector{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}}, y::Vector{DynamicQuantities.Quantity{Float64, Dimensions{DynamicQuantities.FixedRational{Int32, 25200}}}})
      @ LinearAlgebra C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\LinearAlgebra\src\generic.jl:1785
    [6] eval_test(evaluated::Expr, quoted::Expr, source::LineNumberNode, negate::Bool)
      @ Test C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:355
    [7] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:669 [inlined]
    [8] macro expansion
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:164 [inlined]
    [9] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]        
   [10] macro expansion
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:163 [inlined]
   [11] macro expansion
      @ C:\Users\mikei\.julia\juliaup\julia-1.10.0+0.x64.w64.mingw32\share\julia\stdlib\v1.10\Test\src\Test.jl:1577 [inlined]        
   [12] top-level scope
      @ C:\Users\mikei\.julia\packages\LineIntegrals\Y8iYI\test\runtests.jl:128

@mikeingold
Copy link
Contributor

mikeingold commented Jan 20, 2024

Looks like both of those errors stem from isapprox implementation of rtol and atol settings here, I.e.

atol::Real=0, rtol::Real=rtoldefault(x,y,atol)

where rtoldefault here calls real(T).

Edit: I went back and forked this into its own Issue with a MWE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants