PainterQubits / Unitful.jl

Physical quantities with arbitrary units
Other
604 stars 111 forks source link

Either angles or cld don't work consistently #686

Open aplavin opened 11 months ago

aplavin commented 11 months ago

Adding zero shouldn't silently influence results in a quite unexpected way:

julia> round(Int, cld(0+5u"°", 1u"°"))
57

julia> round(Int, cld(5u"°", 1u"°"))
5

Either it should throw an error, or remain consistent. Not sure what part exactly is to blame here: angles handling or the cld method. Probably the former?

sostock commented 11 months ago

The problem is this method: https://github.com/PainterQubits/Unitful.jl/blob/6fb11e7631f9b5c0897e38541ec96c0c7b3a653e/src/quantities.jl#L76

This is intended for AbstractQuantitys that aren’t dimensionless, applying it to dimensionless quantities results in wrong values. It can be fixed by adding a cld(x::Number, y::DimensionlessQuantity) method[^1] that works correctly.

The cld(x::Number, y::AbstractQuantity) method was added in Unitful v1.1 (#317) to make cld(5, 2m) work (I think it was a mistake to add these methods, because they aren’t invariant under unit conversion, but the other maintainers have no problem with them and removing them would be breaking).

[^1]: as well as cld(x::DimensionlessQuantity, y::Number). To resolve ambiguities, it is probably also be necessary to add a cld(x::DimensionlessQuantity, y::DimensionlessQuantity) method. And the same should be done for fld and div.

aplavin commented 11 months ago

If only angles were dimensionful from the beginning... :)

cmichelenstrofer commented 10 months ago

If only angles were dimensionful from the beginning... :)

Dimensionless angles is the SI standard, so it makes sense for Unitful to be the same.

If you want to treat Angles as a Dimension checkout DimensionfulAngles.jl.

aplavin commented 10 months ago

Dimensionless angles is the SI standard

Yes.

it makes sense for Unitful to be the same

Does not follow from the former. In computations, dimensionless angles make it very easy to make mistakes that units are supposed to guard from in the first place. For example, this makes no sense:

julia> 1u"W/°^2" |> u"W"
3282.806350011744 W

If you want to treat Angles as a Dimension checkout DimensionfulAngles.jl.

Nice, but doesn't really help assuming one wants to interoperate with other packages using/defining units. Even taking only units from Unitful.jl itself, DimensionfulAngles doesn't help with distinguishing flux vs intensity:

julia> 1u"lm" == 1u"cd"
true

It's much easier to discard radians after a computation where they were properly tracked than to introduce angular units after a computation where they weren't. astropy deals with it much better, keeping track of angles by default but allowing the user to opt into an "equivalence" where they are treated as dimensionless.

cmichelenstrofer commented 10 months ago

I don't disagree but I respect the decision to be consistent with SI. If anything I would add angular dimensions as an option and SI as default.

DimensionfulAngles doesn't help with distinguishing flux vs intensity

Ah, good point! I missed this (I have never worked with cd or lm...). I will see if it can be added to DimensionfulAngles.

aplavin commented 10 months ago

I'm also all for providing an option for dimensionless/dimensionful angles! But it's important for such an option to work with other packages and functions defining/using units. Like, someone defines

f(x) = ... x ... *u"rad"

and then the enduser should be able to choose whether they want to ignore those radians or not, without changing f. astropy does it successfully.