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

Comparisons with Inf fail #455

Closed
JeffFessler opened this issue Jun 15, 2021 · 10 comments
Closed

Comparisons with Inf fail #455

JeffFessler opened this issue Jun 15, 2021 · 10 comments

Comments

@JeffFessler
Copy link
Contributor

This comparison fails:

1u"m" < Inf
ERROR: DimensionError: 1.0 m and Inf are not dimensionally compatible.

where I think most users would expect to be true because ±Inf should not need to have units.

Now x = 1u"m"; x > 0 also fails but here we have the acceptable alternative x > zero(x).
But there (apparently) is no analog like infinity(x) so we are stuck with Inf.

These are the key lines:

@eval ($i)(x::AbstractQuantity, y::Number) = ($i)(promote(x,y)...)

@eval @inline ($j)(x::AbstractQuantity{T,D1,U1}, y::AbstractQuantity{T,D2,U2}) where {T,D1,D2,U1,U2} = throw(DimensionError(x,y))

That last line could check for ±Inf instead of always throwing an error. The performance hit would be small because that line only gets used in the error condition. Would a PR be welcome?

I search all issue for Inf and found nothing and found just 2 issues that concern isless: #399 #30

@giordano
Copy link
Collaborator

because ±Inf should not need to have units.

Why?

@JeffFessler
Copy link
Contributor Author

Well, why should it have units? And if it should, then why is there no infinity(x) like there is a zero(x)?

The above is a MWE. Here's the real back story.

using Unitful
using Plots

import Dates; d = oneunit(Dates.Day)
x = d * (1:5)
scatter(x, 2x) # works fine

x = 1u"m" * (1:5)
scatter(x, 2x) # fails due to Inf comparison

If a plot vs dates works, so should a plot vs meters...
Of course I could take this specific issue up over at Plots, but I think Unitful would be more user friendly if comparisons with 0 and Inf just worked (even without units), in the same way that Int16(0) < 1.0f0 just works. It might help things just work in other places that make such comparisons, with no obvious downside over an error message.

@giordano
Copy link
Collaborator

Well, why should it have units?

Because it's a number just like all others? I don't see any reasons why it should be special.

then why is there no infinity(x) like there is a zero(x)?

Because no one took the time to do that? But zero and one are important as they are the neutral element of basic arithmetic operations, there is no such argument for infinity

@JeffFessler
Copy link
Contributor Author

Because it's a number just like all others? I don't see any reasons why it should be special.

I disagree that it is "just like all the others" but I'll take this as a no on the PR.
https://en.wikipedia.org/wiki/Extended_real_number_line
Thanks for the replies.

@giordano
Copy link
Collaborator

giordano commented Jun 16, 2021

My point is: if you have an infinite length coming from operations (e.g., 1 / 0u"m"), that does have the units attached and so no problem with that, otherwise why would you expect to be able to compare Inf (a pure Float64) with a length? Why would that be any different from any other Float64?

Do you have a practical case where this is an issue?

@JeffFessler
Copy link
Contributor Author

JeffFessler commented Jun 16, 2021

Why would that be any different from any other Float64?

Inf is different from other "numbers." Try this: Inf == 2Inf. It is true.
There are only 3 cases where x=2x, namely, 0 and Inf and -Inf.
That is at least some rationale for treating 0 and Inf differently.
I do not want to take more of your time debating the meaning of infinity.
https://math.stackexchange.com/questions/36289/is-infinity-a-number
I already described a practical case with a plot above.
Putting infinity aside, you really could make Unitful more user-friendly by allowing comparisons with 0 without insisting on units on 0, in the same way that Base Julia allows one to compare 0//1 with 0.0f0 without forcing the user to do their own type conversion.

Here's the code in case anyone else would find it convenient.

# extended version of _lt that gracefully handles {0, ±Inf} < y
function Unitful._lt(
    x::Unitful.AbstractQuantity{T,NoDims,U1},
    y::Unitful.AbstractQuantity{T,D2,U2},
) where {T,D2,U1,U2}
    y1 = y / oneunit(y)
    x == Inf && (return false) # Inf < y
    x == -Inf && (return -Inf < y1) # -Inf < y
    iszero(x) && (return 0 < y1) # 0 < y
    throw(Unitful.DimensionError(x,y))
end

# extended version of _lt that gracefully handles x < {0, ±Inf}
function Unitful._lt(
    x::Unitful.AbstractQuantity{T,D1,U1},
    y::Unitful.AbstractQuantity{T,NoDims,U2},
) where {T,D1,U1,U2}
    x1 = x / oneunit(x)
    y == -Inf && (return false) # x < -Inf
    y == Inf && (return x1 < Inf) # x < Inf
    iszero(y) && (return x1 < 0) # x < 0
    throw(Unitful.DimensionError(x,y))
end

# Tests:

m = 1u"m"

using Test
@testset "0,Inf" begin
    @test m < Inf
    @test -Inf < m
    @test !(m < -Inf)
    @test !(Inf < m)

    @test Inf > m
    @test m > -Inf
    @test !(m > Inf)
    @test !(-Inf > m)

    @test 0 < m
    @test m > 0
    @test -m < 0
    @test 0 > -m

    @test_throws Unitful.DimensionError m < 2
    @test_throws Unitful.DimensionError 0u"s" < m
end

So the good news here is that with this addition one can support the convenient form 0 < 42u"m" in the way that one would expect, as well as the more rigorous form 0u"m" < 3u"m".

The bad news is that this got the plot example above to get a bit farther along before throwing an error somewhere in uconvert. So I give up for now.

@giordano
Copy link
Collaborator

Inf is different from other "numbers." Try this: Inf == 2Inf. It is true.
There are only 3 cases where x=2x, namely, 0 and Inf and -Inf.
That is at least some rationale for treating 0 and Inf differently.

I honestly fail to see what you want to prove with this argument. In particular, why would you want to strip out the the unit properties from the quantity when comparing with Inf. By your argument we should do the same for 0 which is also a strong no from me. I really don't understand why infinity or zero shouldn't respect the "unit dimensionality" of quantities.

I already described a practical case with a plot above.

Why don't you use UnitfulRecipes.jl?

@JeffFessler
Copy link
Contributor Author

why would you want to strip out the the unit properties

I was not proposing to strip out any properties. I was proposing to also support unit-less comparisons with 0 and Inf, as a convenience to users. I edited the code above to make some corrections. I also edited the text below the code to clarify the benefit. I am closing the issue because it is only a question of convenience, not a bug and convenience is a matter of preference.

Thanks so much for the pointer to UnitfulRecipes! I did not know about that package and it looks like it will address the issue that brought me here in the first place.

giordano pushed a commit that referenced this issue Jun 17, 2021
If I had known this would support plot axes with units, I would not have started down the road to #455.
Maybe this elaboration will help someone else.

[skip ci]
hustf added a commit to hustf/Unitfu.jl that referenced this issue Sep 1, 2022
* Use the `:fancy_exponent` IO context property to override the behavior of fancy exponents (PainterQubits#446)

* Showing how to manually cancel units (PainterQubits#451)

* Showing how to manually cancel units

* Update conversion.md

added the statement that `m/m` is automatically canceled

* Remove contrived examples, fixed typo

* More clear example

This commit changes the expression from which units are shortened to 1km/2.5m, to make clear that the denomenator is part of the expression.

It also fixed a typo (`NoUnit -> NoUnits`)

* Shorten example

It seems like some of the examples did not sit well with some of the contributers. I see their point completely, and in this PR I have made the example minimal and clearer. #LessIsMore

* Added @ref, and more interesting example

I added a reference to the NoUnits type (hopefully - I don't know how to chech that the reference actually works as expected).

I also changed the example to the more complicated one, to make it less trivial and hopefully better showcase the usefullness.

* Update docs/src/conversion.md

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* As per the latest review, which got outdated

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* fix example `isa(1m, Length)` (PainterQubits#454)

`isa(1m, Length)` did not work after `using Unitful` but the corrected version works.

* Elaborate on UnitfulRecipes in readme (PainterQubits#456)

If I had known this would support plot axes with units, I would not have started down the road to PainterQubits#455.
Maybe this elaboration will help someone else.

[skip ci]

* Add link to UnitfulBuckinghamPi in README (PainterQubits#442)

I just registered the [UnitfulBuckinghamPi.jl](https://github.com/rmsrosa/UnitfulBuckinghamPi.jl) package. It solves for the adimensional groups in a list of Unitful parameters (quantities, units, dimensions, or combinations thereof).

This PR adds a link to the package in the README.

[skip ci]

* Fix doctests (PainterQubits#464)

* deg2rad and rad2deg with "units" (PainterQubits#459)

* deg2rad and rad2deg

* v1.9

* Update src/pkgdefaults.jl

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* Update README.md

Mentioned NaturallyUnitful.jl

* Update .gitignore (PainterQubits#482)

* Update .gitignore

* Update .gitignore

Removed binary stuff

[skip ci]

* Use aggressive constprop for `^(::AbstractQuantity, ::Rational)` (PainterQubits#487)

* Calculate correct `eltype` when multiplying `StepRangeLen` by `Units`. (PainterQubits#485)

* Prevent promotion of StepRangeLen eltype when multiplying by unit

* Release 1.9.1

* Fix multiplication of range and quantity (PainterQubits#489)

* Remove unnecessary Bool-AbstractQuantity multiplication methods (PainterQubits#491)

* made Bool-Quantity multiplication symmetric

* added tests for Bool-Quantity multiplication

* Remove unnecessary multiplication methods for Bool, AbstractQuantity

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Remove comment from previous Bool, AbstractQuantity methods

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Add NaN, -Inf tests for multiplication by false

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Resolve method ambiguity (PainterQubits#495)

* Fix fastmath trig functions (PainterQubits#497)

* make `norm` use `norm` rather than `abs` (PainterQubits#500)

* make `norm` use `norm` rather than `abs`

* support broadcasting on ranges (PainterQubits#501)

* Attempt to allow preferunits to work with non-pure units (PainterQubits#478)

* Attempt to allow preferunits to work with non-pure units

Should fix PainterQubits#457. Prior to this change, attempts to use something that
looked like "preferunits(C/ms)" would result in strange behavior, without
throwing any sort of errors. Now, that should work nicely.

* Warn user when preferunits causes redundant units

Issue a warning when preferunits creates redundant units, which could stop
it from sucessfully simplifying certain quantities.

* Fix new upreferred behavior for non-dimensional quantities

* Add tests for preferunits changes

* Update test/runtests.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Fix preferunits tests

* Apply suggestions from code review

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* Fix issues added by removing excess arrays

* No longer use string for key while checking for units of different scales

* Update test/runtests.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Update runtests.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* Unit construction macro now defines docs for units (PainterQubits#476)

* Unit construction macro now defines docs for units

See PainterQubits#436

* Add documentation capabilities to most defined macros

This lets all unit and dimension definition macros repect doc strings
set in unit definition. Additionally, it automatically defines
documentation for derived units using power-of-ten prefixes.

* Add documentation for some units in pkgdefaults

Documentation has been added for all base units and many other SI units, as
well as adding a special case to the documentation so that kg and
documented as being the SI base unit.

* Allow documentation for log scales and units

* Rewrite documentation format for all units through "Liter"

Additionally, changed how the prefixed unit documentation is written; this
now means that prefixed units made by other modules should be properly
documented (tested with UnitfulAstro).

* Added documentation for all remaining units

Documentation is still required for log scales

* Update src/pkgdefaults.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Update src/pkgdefaults.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Update src/pkgdefaults.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Update src/pkgdefaults.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Apply suggestions from code review

Added descriptions of why certain alternate symbols are used (q for electron charge, minute for minutes)

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Remove excess newlines; add names to docstrings

* Add NoDims documentation

* Standardized number and equation format among quantity and unit docs

* Standardize format of dimensions in unit docs

* Apply suggestions from code review

Various minor fixes and changes, mostly typos and format changes.

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Switch from inline to block code blocks in new docstrings.

Also standardize docstrings to `2π` for 2 pi.

* Add automatic documentation for everything defined by the dimension macro

Also removed documentation from the pieces of log units that currently
don't have any sort of automatic documentation generation system in place.

* Fix documentation issues with two different symbols for vacuum permittivity constant.

* Minor formatting fixes

* Use block quotes for prefixed units' doc strings

* Update src/types.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Make automatic documentation optional

Also (optionally) generate automatic documentation for derived dimensions.

* Add dimension info in automatic prefixed unit docs.

* Change method for adding dimensions to prefixed unit documentation

Now no longer tries to `eval` stuff while running the macro.

* Fixes to documentation and code structure for autodocs for various macros

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Various format fixes and liters documentation changes.

* Fix missing optional argument causing test failure

* Remove no-longer-necessary NullLogger for liter and kilogram docs

* Remove unnecessary autodocs arguments for non-SI-prefixed units

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Fix invalid links being created in non-Unitful documentation

* Add spaced out column for autogen parameter where possible

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Update src/user.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Add tests for autodocs

* Add test for prefixed reference units

* Hopefully fix docstrings and docstring tests on Julia-1.0

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* More documentation changes from code review

Also removed documentation ability for logunit and logscale (as these should be another pull request)

* Apply suggestions from code review

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* fix range eltype (PainterQubits#503)

* Fix some major invalidations to improve compile times (PainterQubits#509)

* Fix some major invalidations to improve compile times

Unitful.jl was flagged as a major invalidator of compilation downstream. Test:

```julia
# From: https://timholy.github.io/SnoopCompile.jl/stable/snoopr/
using SnoopCompile
invalidations = @Snoopr begin
    using DifferentialEquations

    function lorenz(du,u,p,t)
     du[1] = 10.0(u[2]-u[1])
     du[2] = u[1]*(28.0-u[3]) - u[2]
     du[3] = u[1]*u[2] - (8/3)*u[3]
    end
    u0 = [1.0;0.0;0.0]
    tspan = (0.0,100.0)
    prob = ODEProblem(lorenz,u0,tspan)
    alg = Rodas5()
    tinf = solve(prob,alg)
end;

trees = SnoopCompile.invalidation_trees(invalidations);

@show length(SnoopCompile.uinvalidated(invalidations)) # show total invalidations

show(trees[end]) # show the most invalidated method
```

This method won the prize for the absolute most invalidations 🎉. But I think the bigger issue is that it simply doesn't follow Julia semantics. It fixes the types for issue PainterQubits#127 in a way that gives a stricter type than Julia would do in the normal cases (which is why the invalidation occurs).

After this PR, heterogeneous arrays of numbers with Quantity in there act normally, and compile times are back to normal. Here's a showcase of it being normal:

```julia

using Unitful, Test
m = u"m"
cm = u"cm"

b = Union{Complex,Float64}[0 + 0im, 0.0]
@test b + b == b
@test b .+ b == b
@test eltype(b+b) === Number

b = Number[0 + 0im, 0.0]
@test b + b == b
@test b .+ b == b
@test eltype(b+b) === Number

b = [0.0, 0.0m]
@test b + b == b
@test b .+ b == b
@test eltype(b+b) === Number
```

* Update promotion.jl

* Release v1.10.0

* Fix PainterQubits#465 for isapprox with complex arrays (PainterQubits#468)

* deg2rad and rad2deg

* v1.9

* Update src/pkgdefaults.jl

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* fix PainterQubits#465

* test, v1.9.1

* Update src/quantities.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* positive test

* Update test/runtests.jl

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Fix `range` implementation on Julia master and resolve method ambiguities (PainterQubits#514)

* _range(start, ::Nothing, ::Nothing, len)

* _range(::Nothing, ::Nothing, stop, len)

* Add tests for ranges with complex elements

* Use modern  syntax

* Remove unnecessary `<:Real`

* Fix range(start::Quantity{BigFloat}; step::Quantity{BigFloat}; length)

* Fix for missing _rangestyle on 1.8

* Remove `real`

* Use `step` everywhere (instead of `st`)

* Fix range(;step, stop, len) on Julia master

* Fix range(;stop, step, length) for mixed number/quantity

* Add some newlines and comments

* Rename _range_step_stop_length → _unitful_step_stop_length

To avoid confusion with Base.range_step_stop_length

* Refactor _range(start, ::Nothing, stop, length)

* Rename 3-arg `Base._range` → `_unitful_start_stop_length` (`Base._range` always has 4 arguments)
* Move promotion to `_unitful_start_stop_length`

* Refactor _range(start, step, ::Nothing, length)

* Resolve method ambiguity on Julia ≥ 1.7

* Simplify implementation

* Use modern `where` syntax for `colon`

* Add two-argument colon for dimensionless quantities

* Remove unnecessary code related to encoding of μ (PainterQubits#511)

* Add tests for encoding of μ

* Remove unnecessary code

* Use U+03BC everywhere

* Same changes for U+025B/U+03B5

* Fix printing of `StepRangeLen` with complex elements (PainterQubits#513)

* Enable `zero` for arrays with non-concrete eltype (PainterQubits#516)

* Release 1.11.0 (PainterQubits#519)

* Delete export of `convertr`, `convertrp` (PainterQubits#530)

* Delete export of `convertr`

There is no `convertr`, so don't export it.

* Don't export convertrp

It's not defined

* Add `cispi` and `sincospi` (PainterQubits#533)

* Add `modf` (PainterQubits#539)

* Add `modf` for dimensionless quantities

* Add tests for `modf`

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>

* Add link to UnitfulChainRules.jl to README (PainterQubits#542)

* modified:   Project.toml v1.10.1 reflect Unitful, add ConstructionBase
modified:   src/pkgdefaults.jl   Use new macros, deg rad change
modified:   test/runtests.jl     Add ConstructionBase

* modified:   Project.toml   v1.11.0

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
Co-authored-by: KronosTheLate <61620837+KronosTheLate@users.noreply.github.com>
Co-authored-by: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Co-authored-by: Jeff Fessler <JeffFessler@users.noreply.github.com>
Co-authored-by: Ricardo Rosa <rmsrosa@gmail.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mo8it <76752051+Mo8it@users.noreply.github.com>
Co-authored-by: Samuel Buercklin <SBuercklin@users.noreply.github.com>
Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
Co-authored-by: Alexander <alexander@plav.in>
Co-authored-by: Luke Bemish <lukebemish@gmail.com>
Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
Co-authored-by: adkabo <61999878+adkabo@users.noreply.github.com>
Co-authored-by: Rashid Rafeek <rashidrafeek@gmail.com>
@t-bltg
Copy link

t-bltg commented Sep 21, 2022

Regarding the recent issue in Plots (JuliaPlots/Plots.jl#4366), I'd say that a finite quantity should be comparable to +/- Inf, and would tend towards @JeffFessler 's reasoning and wish to open a PR to fix this.

@cstjean
Copy link
Contributor

cstjean commented Sep 22, 2022

On purely math ground, it'd be great to have 5m > 0, since 0m = 0, but consider the expression some_length + some_mass. It could return either a Length, or a Mass, depending on which one of the two is 0. That's type unstable and simply unthinkable for the performance of Unitful.

Besides, I agree with @giordano that while "no apple" and "no elephant" is the same thing, adding "infinitely many apples" and "infinitely many elephants" is nonsensical. You can write Inf * u"m" and it works fine.

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

No branches or pull requests

4 participants