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

Inconsistent behavior of Base.floatrange #45336

Closed
metab0t opened this issue May 17, 2022 · 6 comments
Closed

Inconsistent behavior of Base.floatrange #45336

metab0t opened this issue May 17, 2022 · 6 comments
Labels
domain:ranges Everything AbstractRange kind:bug Indicates an unexpected problem or unintended behavior

Comments

@metab0t
Copy link
Contributor

metab0t commented May 17, 2022

When digging into a bug in StatsBase.fit:

I find that the core issue happens in usage of Base.floatrange

I try the following code:

julia> Base.floatrange(1.05 , 1.0 , 2, 1.0)
2.05:2.0:4.05

julia> Base.floatrange(1.0  , 1.0 , 2, 1.0)
1.0:1.0:2.0

In twiceprecision.jl, ref in two steprangelen_hp definitions has different meanings.

  • In the integer case, it means TwicePrecision{Float64}(ref)
  • In the float case, it means TwicePrecision{Float64}(ref...) (it should align with integer case)

However, TwicePrecision{Float64}(1.0, 1.0) and TwicePrecision{Float64}((1.0, 1.0))are totally different.

Therefore, two floatrange definitions return different results

Code examples:

julia> Base.floatrange(Float64, 11, 10, 2, 1)
11.0:10.0:21.0

julia> Base.floatrange(11.0, 10.0, 2, 1.0) # fallback to use floatrange(Float64, ::Integer, ::Integer, ::Integer, ::Integer)
11.0:10.0:21.0

julia> Base.floatrange(11.1, 10.0, 2, 1.0) # use float case of steprangelen_hp
12.1:11.0:23.1

Version info:

julia> versioninfo()
Julia Version 1.7.2
Commit bf53498635 (2022-02-06 15:21 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: AMD Ryzen 7 5800H with Radeon Graphics
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, znver3)
Environment:
  JULIA_NUM_THREADS = auto
  JULIA_PKG_USE_CLI_GIT = true
@oscardssmith
Copy link
Member

wow. That is awful.

@oscardssmith oscardssmith added kind:bug Indicates an unexpected problem or unintended behavior domain:ranges Everything AbstractRange labels May 17, 2022
@N5N3
Copy link
Member

N5N3 commented May 18, 2022

Do we have a MWE to reproduce this bug with public API? (Base.floatrange is not documented)

I build the latest master with L395 commented. ranges test only show one error

Test Summary: |     Pass  Error  Broken     Total     Time
  Overall     | 12110865      1  327682  12438548  1m11.8s
    ranges    | 12110865      1  327682  12438548  1m10.3s
    FAILURE

The global RNG seed was 0xd80800aebdb44d7a3931f2e34a3be386.

Error in testset ranges:
Error During Test at C:\Users\XXX\Documents\Github\julia\test\ranges.jl:877
  Test threw exception
  Expression: length(Base.floatrange(-3.0e9, 1.0, 1, 1.0)) == 1
  MethodError: no method matching floatrange(::Float64, ::Float64, ::Int64, ::Float64)
  Closest candidates are:
    floatrange(::Type{T}, ::Integer, ::Integer, ::Integer, ::Integer) where T at twiceprecision.jl:381

So I think we should just remove it.

@metab0t
Copy link
Contributor Author

metab0t commented May 18, 2022

https://github.com/search?l=Julia&p=1&q=Base.floatrange&type=Code

After code search on GitHub, we can almost conclude that the only widely user of Base.floatrange is StatsBase/src/hist.jl.
So maybe we should remove it and fix StatsBase?

ararslan pushed a commit to JuliaStats/StatsBase.jl that referenced this issue May 20, 2022
* Replace Base.floatrange with simplified implementation
Base.floatrange has bugs and is not public API anymore
See JuliaLang/julia#45336
Use a simplified implementation instead

* Inline floatrange
@metab0t
Copy link
Contributor Author

metab0t commented May 20, 2022

Maybe we can remove it because StatsBase.jl does not depend on it anymore.

@Moelf
Copy link
Sponsor Contributor

Moelf commented Jun 4, 2022

@metab0t feel free to make a PR implementing it and we can PkgEval it

@metab0t
Copy link
Contributor Author

metab0t commented Jun 4, 2022

@Moelf I have opened a PR to remove the inconsistent instance of Base.floatrange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ranges Everything AbstractRange kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants