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

unique fails with column-type FixedDecimal #3418

Closed
baumgold opened this issue Jan 18, 2024 · 5 comments · Fixed by #3419
Closed

unique fails with column-type FixedDecimal #3418

baumgold opened this issue Jan 18, 2024 · 5 comments · Fixed by #3419
Labels
bug ecosystem Issues in DataFrames.jl ecosystem
Milestone

Comments

@baumgold
Copy link

baumgold commented Jan 18, 2024

Trying to call unique on a DataFrame that contains a column of type FixedDecimal fails with a MethodError:

julia> using DataFrames, FixedPointDecimals

julia> df = DataFrame(A = [FixedDecimal{Int,3}(1), FixedDecimal{Int,3}(2), FixedDecimal{Int,3}(3)])
3×1 DataFrame
 Row │ A
     │ FixedDec
─────┼───────────
   11.0
   22.0
   33.0

julia> unique(df)
ERROR: MethodError: no method matching big(::FixedDecimal{Int64, 3})

Closest candidates are:
  big(::Type{Union{}}, Any...)
   @ Base number.jl:394
  big(::Type{<:Integer})
   @ Base gmp.jl:490
  big(::Type{<:AbstractFloat})
   @ Base mpfr.jl:416
  ...

Stacktrace:
  [1] refpool_and_array(x::Vector{FixedDecimal{Int64, 3}})
    @ DataFrames ~/.julia/packages/DataFrames/58MUJ/src/groupeddataframe/utils.jl:159
  [2] _broadcast_getindex_evalf
    @ Base.Broadcast ./broadcast.jl:709 [inlined]
  [3] _broadcast_getindex
    @ Base.Broadcast ./broadcast.jl:682 [inlined]
  [4] #31
    @ Base.Broadcast ./broadcast.jl:1118 [inlined]
  [5] ntuple
    @ Base ./ntuple.jl:48 [inlined]
  [6] copy
    @ Base.Broadcast ./broadcast.jl:1118 [inlined]
  [7] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.Style{Tuple}, Nothing, typeof(DataFrames.refpool_and_array), Tuple{Tuple{Vector{FixedDecimal{}}}}})
    @ Base.Broadcast ./broadcast.jl:903
  [8] nonunique(df::DataFrame; keep::Symbol)
    @ DataFrames ~/.julia/packages/DataFrames/58MUJ/src/abstractdataframe/unique.jl:94
  [9] nonunique
    @ DataFrames ~/.julia/packages/DataFrames/58MUJ/src/abstractdataframe/unique.jl:86 [inlined]
 [10] #unique#192
    @ DataFrames ~/.julia/packages/DataFrames/58MUJ/src/abstractdataframe/unique.jl:292 [inlined]
 [11] unique(df::DataFrame)
    @ DataFrames ~/.julia/packages/DataFrames/58MUJ/src/abstractdataframe/unique.jl:290
 [12] top-level scope
    @ REPL[3]:1
Some type information was truncated. Use `show(err)` to see complete types.

Note this may well be a problem with FixedPointDecimals.jl rather than DataFrames.jl. Version and environment information:

julia> versioninfo()
Julia Version 1.10.0
Commit 3120989f39b (2023-12-25 18:01 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × Intel(R) Xeon(R) Gold 6136 CPU @ 3.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, skylake-avx512)
  Threads: 5 on 4 virtual cores
Environment:
  JULIA_NUM_THREADS = 4

(@v1.10) pkg> status DataFrames, FixedPointDecimals
Status `~/.julia/environments/v1.10/Project.toml`
  [a93c6f00] DataFrames v1.6.1
  [fb4d412d] FixedPointDecimals v0.5.2
@bkamins bkamins added the ecosystem Issues in DataFrames.jl ecosystem label Jan 18, 2024
@bkamins
Copy link
Member

bkamins commented Jan 18, 2024

@nalimilan - we need to make a decision here. FixedPointDecimals.jl does not support conversion of integer FixedDecimal values to BigInt. I think it should, but the fact is that it does not. Our code assumes that all integers can be passed to big and be properly converted to BigInt. Do you think we should change this assumption?

@baumgold - the simplest fix would be on FixedPointDecimals.jl side if they added support for conversion of integer values to BigInt.

@baumgold
Copy link
Author

@bkamins - thanks for the quick response. I would be happy with a fix on the FixedPointDecimals.jl side. I'm not familiar enough with the purpose of Base.big to know how that should be implemented for FixedDecimal. You say that it should support conversion to BigInt but FixedDecimal is more akin like a non-lossy BigFloat than a BigInt. How should we proceed? Thanks for your help.

@baumgold
Copy link
Author

It looks like the following may be the correct implementation but I'm not confident. Thoughts?

julia> Base.big(n::FixedDecimal) = convert(BigFloat, n)

julia> Base.big(::Type{<:FixedDecimal}) = BigFloat

@bkamins
Copy link
Member

bkamins commented Jan 18, 2024

@baumgold - it is not the type of the numbers you use but the actual value. If your values were not actually all integers things would work:

julia> df = DataFrame(A = [FixedDecimal{Int,3}(1), FixedDecimal{Int,3}(2), FixedDecimal{Int,3}(3.1)])
3×1 DataFrame
 Row │ A
     │ FixedDec…
─────┼───────────
   1 │       1.0
   2 │       2.0
   3 │       3.1

julia> unique(df)
3×1 DataFrame
 Row │ A
     │ FixedDec…
─────┼───────────
   1 │       1.0
   2 │       2.0
   3 │       3.1

Note that BigInt works:

julia> BigInt(FixedDecimal{Int,3}(1))
1

It is just the shorthand big does not work (which could be added to that package but probably its maintainers should make this decision).

@nalimilan - probably we can change big to BigInt in DataFrames.jl as a solution. OK?


@baumgold - as a temporary fix you can define big(x::FixedDecimal) = BigInt(x) in your code. It should not break anything (as big is not supported by FixedDecimal).

@baumgold
Copy link
Author

I logged JuliaMath/FixedPointDecimals.jl#91 to request that Base.big be implemented for FixedDecimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ecosystem Issues in DataFrames.jl ecosystem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants