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

Either angles or cld don't work consistently #686

Open
aplavin opened this issue Oct 18, 2023 · 6 comments
Open

Either angles or cld don't work consistently #686

aplavin opened this issue Oct 18, 2023 · 6 comments
Labels
bug dimensionless dimensionless quantities/units (mm/m, rad, …) and their interaction with plain numbers

Comments

@aplavin
Copy link
Contributor

aplavin commented Oct 18, 2023

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 sostock added bug dimensionless dimensionless quantities/units (mm/m, rad, …) and their interaction with plain numbers labels Oct 18, 2023
@sostock
Copy link
Collaborator

sostock commented Oct 18, 2023

The problem is this method:

($f)(x::Number, y::AbstractQuantity) = Quantity(($f)(x, ustrip(y)), unit(x) / unit(y))

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) method1 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).

Footnotes

  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
Copy link
Contributor Author

aplavin commented Oct 18, 2023

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

@cmichelenstrofer
Copy link
Contributor

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
Copy link
Contributor Author

aplavin commented Nov 26, 2023

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
Copy link
Contributor

cmichelenstrofer commented Nov 27, 2023

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
Copy link
Contributor Author

aplavin commented Nov 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dimensionless dimensionless quantities/units (mm/m, rad, …) and their interaction with plain numbers
Projects
None yet
Development

No branches or pull requests

3 participants