Constructor with units #305

SebastianRuffert · 2022-06-15T07:01:27Z

No description provided.

SebastianRuffert · 2022-06-15T07:05:44Z

This is a very straightforward way to implement units. Do we want to allow for units in both constructor functions or only the one I changed so far?

SebastianRuffert · 2022-06-15T07:12:11Z

Also I am unsure if we should convert all default values to the user-specified unit, if only one keyword argument is given

fhagemann · 2022-06-15T07:28:07Z

src/ConstructiveSolidGeometry/VolumePrimitives/Box.jl

+function Box(CO, hX::Quantity, hY::Quantity, hZ::Quantity, origin, rotation) 
+    _hX = to_internal_units(hX)
+    _hY = to_internal_units(hY)
+    _hZ = to_internal_units(hZ)
+    Box(CO, _hX, _hY, _hZ, origin, rotation)
+end
+
 function Box(::Type{CO}=ClosedPrimitive;
-    hX = 1,
-    hY = 1,
-    hZ = 1,
+    hX = 1u"m",
+    hY = 1u"m",
+    hZ = 1u"m",


SSD does not know which function to call.
Try to specify in the already existing function above this one that TX, TY and TZ can only be a Real.

SebastianRuffert · 2022-06-15T09:33:56Z

src/ConstructiveSolidGeometry/VolumePrimitives/Box.jl

@@ -70,6 +70,15 @@ function Box(CO, hX::TX, hY::TY, hZ::TZ, origin::PT, rotation::ROT) where {TX, T
    Box{T}(CO,T(hX), T(hY), T(hZ), origin, rotation)
 end

+#Unit convesion
+Box(CO, hX::Quantity, hY, hZ, origin, rotation) = Box(CO, to_internal_units(hX), hY, hZ, origin, rotation)


Is there a convenient way to write those permutations?

Use RealQuantity?

Is RealQuantity part of Unitful? I can not find it

RealQuantity is part of SSD:

SolidStateDetectors.jl/src/Units.jl

Line 90 in 9dabbf6

const RealQuantity = MaybeWithUnits{<:Real}

fhagemann · 2022-06-23T11:02:16Z

src/ConstructiveSolidGeometry/VolumePrimitives/Box.jl

+Box(CO, hX, hY::Quantity, hZ, origin, rotation) = Box(CO, hX, to_internal_units(hY), hZ, origin, rotation)
+Box(CO, hX, hY, hZ::Quantity, origin, rotation) = Box(CO, hX, hY, to_internal_units(hZ), origin, rotation)
+Box(CO, hX::Quantity, hY::Quantity, hZ, origin, rotation) = Box(CO, to_internal_units(hX), to_internal_units(hY), hZ, origin, rotation)
+Box(CO, hX, hY::Quantity, hZ::Quantity, origin, rotation) = Box(CO, hX, to_internal_units(hY), to_internal_units(hZ), origin, rotation)
+Box(CO, hX::Quantity, hY, hZ::Quantity, origin, rotation) = Box(CO, to_internal_units(hX), hY, to_internal_units(hZ), origin, rotation)
+Box(CO, hX::Quantity, hY::Quantity, hZ::Quantity, origin, rotation) = Box(CO, to_internal_units(hX), to_internal_units(hY), to_internal_units(hZ), origin, rotation)


I am not a fan of these permutations...

What is the problem here and why doesn't this work without permutations?
to_internal_units also works for numbers without units and just returns the number:

SolidStateDetectors.jl/src/ConstructiveSolidGeometry/Units.jl

Line 8 in 9dabbf6

to_internal_units(x::Real) = x

This could be fixed by RealQuantity but I see that RealQuantity is only defined in SSD and not CSG and we are modifying CSG functions. Maybe it makes sense to define RealQuantity in CSG already and export it?

Then, the following might (?) work:

function Box(CO, hX::RealQuantity, hY::RealQuantity, hZ::RealQuantity, origin, rotation) _hX = to_internal_units(hX) _hY = to_internal_units(hY) _hZ = to_internal_units(hZ) Box(CO, _hX, _hY, _hZ, origin, rotation) end

fhagemann · 2022-06-23T11:04:10Z

src/ConstructiveSolidGeometry/PointsAndVectors/Points.jl

@@ -109,6 +109,8 @@ end
 @inline _convert_point(pt::AbstractCoordinatePoint, ::Type{Cylindrical}) = CylindricalPoint(pt)
 @inline _convert_point(pt::AbstractCoordinatePoint, ::Type{Cartesian}) = CartesianPoint(pt)

+scale(cart::CartesianPoint{T}, fact) where T = CartesianPoint{T}(cart.x*fact, cart.y*fact, cart.z*fact)


I would be careful with this!
If we look at the docstring, CartesianPoint will always be saved in units of m.
I would prefer to have a constructor with units for CartesianPoint and solve this via the constructor and not with scale..

fhagemann · 2022-06-23T11:05:08Z

src/ConstructiveSolidGeometry/VolumePrimitives/Box.jl

-    Box{T}(CO, hX, hY, hZ, origin, rotation)
+) where {T, CO, UN<:Unitful.Units}
+    unit_factor = ustrip(uconvert(unt,1u"m"))
+    Box{T}(CO, hX/unit_factor, hY/unit_factor, hZ/unit_factor, scale(CartesianPoint{T}(origin),1/unit_factor), rotation)


(origin) here does not need brackets, but as said above: maybe there is another way of implementing this.

lmh91 · 2022-06-28T12:54:20Z

I don't think we should do this like that.

Let's take just the CartesianPoint as an example (its the same for everything else including the primitives):

CartesianPoint(1u"m", 2u"m", 3u"m")

should return CartesianPoint{promote_type(typeof.(x, y, y))} (pseudo code) and definitely not convert
to CartesianPoint{Int/Float}.

Instead we have to add methods to Unitful.ustrip and Unitful.uconvert for our types.
uconvert might be a bit difficult as our types have different fields. I am not sure if there is a general syntax for that in Unitful.jl.
Maybe there is also a function like unify_units in Unitful.jl for cases like that. Or this should be handled in the promotion.
E.g. (1u"m", 2u"mm)" will be promoted to (1.0u"m", 0.002u"m").
We have to look into Unitful.jl if there are already some conventions for this.

If not, we could (in SSD but not in the submodule CSG), define some _ssd_uconvert-methods for our types and
then just ustrip them with the ustrip methods which we can define in the CSG submodule.

lmh91 · 2022-06-28T12:57:09Z

I just tested on the current main branch:

julia> CartesianPoint(1u"m", 2u"m", 3u"m")
3-element CartesianPoint{Quantity{Int64, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}} with indices SOneTo(3):
 1 m
 2 m
 3 m

That works already out of the box as we do not limit T for AbstractGeometry.

julia> CartesianPoint(1u"m", 2u"m", 3u"mm")
3-element CartesianPoint{Quantity{Rational{Int64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}} with indices SOneTo(3):
    1//1 m
    2//1 m
 3//1000 m

And there seems also already to be some promotion rules defined in Unitful.jl as the second case also works :)

lmh91

Hmm, this topic is a bit tricky.

In principle, we (Julia ecosystem) don't want to convert a Quantity by default to its underlying type. This is should be done by implementing a method for ustrip for Box.
So a primitive with units could be constructed (and the information of the units
are still attached to the primitive and not removed like it is implemented right now).

In order to do this we would need to change the structs of the primitives though.
We would need to separate the type of the rotation from the types of the other fields
like hX:

struct Box{T,CO,RT} <: AbstractVolumePrimitive{T,CO}
    hX::T # so `T` could also be a Quantity here
    hY::T
    hZ::T
    origin::CartesianPoint{T}
    rotation::SMatrix{3,3,RT,9}
end

We also would need to change the definition of the CylindricalPoint by the way
as we will need different types for φ and r&z.

This would be in principle the best (julianic) way in my opinion.
However, it requires the most work as well and make things a bit more complicated (more parametric), though not impossible.

Or, we force the ustrip in the construction and make it impossible to construct primitives with units still attached like it is the design in this PR right now.
But, then we should also do this for the point types like CartesianPoint. For consistency.
And for the points, I would really like to have them with units attached 🤔

Hmm...

fhagemann · 2022-08-01T05:31:33Z

src/ConstructiveSolidGeometry/ConstructiveSolidGeometry.jl

    _csg_convert_args(eltype::Type{T}, r::Tuple) where T = broadcast(x -> _csg_convert_args(T, x), r)
    _csg_convert_args(eltype::Type{T}, r::Nothing) where T = nothing
+    _csg_convert_args(eltype::Type{T}, r::Quantity) where T = convert(T,ustrip(uconvert(u"m",r)))


This only works for LengthQuantity, not for angles etc.

fhagemann · 2022-08-09T17:48:11Z

src/ConstructiveSolidGeometry/ConstructiveSolidGeometry.jl

+    _precision_type(::Type{T}) where {T <: Real} = T
+    _precision_type(::Type{Quantity{T}}) where {T <: Real} = T
+
+    float(::Type{Quantity{T}}) where T = T


Careful! This is type piracy!
Quantity is a type that SSD does not own, so we should be VERY careful when extending the Base.float method

fhagemann · 2022-08-09T17:49:10Z

test/runtests.jl

+# @testset "Comparison to analytic solutions" begin
+#     include("comparison_to_analytic_solutions.jl")
+# end
+# 
+# @testset "SOR GPU Backend" begin
+#     include("SOR_GPU_Backend.jl")
+# end


Please don't exclude tests without mentioning why you exclude them.

fhagemann · 2022-08-09T17:50:02Z

src/ConstructiveSolidGeometry/ConstructiveSolidGeometry.jl

@@ -38,7 +38,7 @@ module ConstructiveSolidGeometry
    const CartesianTicksTuple{T} = NamedTuple{(:x,:y,:z), NTuple{3,Vector{T}}}
    const CylindricalTicksTuple{T} = NamedTuple{(:r,:φ,:z), NTuple{3,Vector{T}}}

-    abstract type AbstractGeometry{T <: AbstractFloat} end
+    abstract type AbstractGeometry{T <: Number} end


Why Number and not AbstractFloat?

SebastianRuffert marked this pull request as draft June 15, 2022 07:01

fhagemann reviewed Jun 15, 2022

View reviewed changes

SebastianRuffert commented Jun 15, 2022

View reviewed changes

SebastianRuffert force-pushed the BoxwithUnits branch from 50680e0 to bdf53b9 Compare June 21, 2022 07:52

SebastianRuffert requested review from fhagemann and lmh91 June 23, 2022 06:30

fhagemann reviewed Jun 23, 2022

View reviewed changes

SebastianRuffert added 9 commits July 7, 2022 09:10

Add unit conversion

47c2294

Other tries

9e2c587

Unit conversion using permutations of argument types

fe3ad82

Add constructor with units as function argument

275b0c1

Add scale function for CartesianPoint

71a38d7

Adapt Geometry to unit option

efd89fc

Enable constructor with units for Box

5e6ee6e

Add conversion and promotion functions for Quantities

ea697b2

Use units in tests

3c62d0b

SebastianRuffert force-pushed the BoxwithUnits branch from b688a67 to 3c62d0b Compare July 7, 2022 10:28

SebastianRuffert marked this pull request as ready for review July 7, 2022 12:09

SebastianRuffert requested a review from fhagemann July 11, 2022 06:26

lmh91 reviewed Jul 11, 2022

View reviewed changes

fhagemann reviewed Aug 1, 2022

View reviewed changes

Add Box with Quantity

56e4fc9

SebastianRuffert marked this pull request as draft August 4, 2022 07:52

SebastianRuffert added 2 commits August 4, 2022 09:53

Type promotion with quantities

783bf9f

Split precision type and T in Box

8278c9f

fhagemann reviewed Aug 9, 2022

View reviewed changes

SebastianRuffert added 3 commits August 16, 2022 14:48

Fix bugs

dd4d9e6

Fix type piracy

6126087

Adapt tests to box with units

772bbdb

SebastianRuffert marked this pull request as ready for review August 16, 2022 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constructor with units #305

Constructor with units #305

SebastianRuffert commented Jun 15, 2022

SebastianRuffert commented Jun 15, 2022

SebastianRuffert commented Jun 15, 2022

fhagemann Jun 15, 2022

SebastianRuffert Jun 15, 2022

fhagemann Jun 15, 2022

SebastianRuffert Jun 21, 2022

fhagemann Jun 23, 2022

fhagemann Jun 23, 2022

fhagemann Jun 23, 2022

fhagemann Jun 23, 2022

lmh91 commented Jun 28, 2022 •

edited

Loading

lmh91 commented Jun 28, 2022 •

edited

Loading

lmh91 left a comment

fhagemann Aug 1, 2022

fhagemann Aug 9, 2022

fhagemann Aug 9, 2022

fhagemann Aug 9, 2022

Constructor with units #305

Are you sure you want to change the base?

Constructor with units #305

Conversation

SebastianRuffert commented Jun 15, 2022

SebastianRuffert commented Jun 15, 2022

SebastianRuffert commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmh91 commented Jun 28, 2022 • edited Loading

lmh91 commented Jun 28, 2022 • edited Loading

lmh91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmh91 commented Jun 28, 2022 •

edited

Loading

lmh91 commented Jun 28, 2022 •

edited

Loading