permit more than 256 discrete levels #1186
Conversation
src/misc.jl
Outdated
| discretize_make_ia(values::AbstractVector, levels) = | ||
| IndirectArray(Array{UInt8}(indexin(values, levels)), levels) | ||
| discretize_make_ia(values::AbstractVector, levels::AbstractVector{T}) where T = | ||
| IndirectArray{T,1,AbstractVector{UInt32},AbstractVector{T}}(indexin(values, levels), levels) |
There was a problem hiding this comment.
Shouldn't using UInt work here and letting Julia pick the optimal bitstype?
There was a problem hiding this comment.
UInt is an alias for UInt64
There was a problem hiding this comment.
The AbstractVector type parameters look weird. Wouldn't IndirectArray{UInt32}(...) be enough here?
|
@andreasnoack @nalimilan could you please review? CI and regression tests pass. just need to add a test. |
|
I'm assuming this doesn't fix the first issue: #1130 (comment). That is, where you have a dataset with p levels, but you use the |
|
correct @Mattriks . that's more of a design decision, and i'm just trying to fix the regression you found later. |
Codecov Report
@@ Coverage Diff @@
## master #1186 +/- ##
=======================================
Coverage 87.28% 87.28%
=======================================
Files 34 34
Lines 4049 4049
=======================================
Hits 3534 3534
Misses 515 515
Continue to review full report at Codecov.
|
|
@bjarthur Have you noticed my comment? |
|
The issue for me is that the ability to specify fewer levels than available levels worked in Gadfly 0.6.5, but doesn't work on master. As #1130 (comment) does not work on master than this statement in the docs: |
|
@nalimilan i did, yes. turns out note that this PR uses a boiled down test which only captures the plan is to merge this once CI passes and then start working on 0.7, which is the most pressing issue now in my mind. |
a partial solution to #1130 (comment)
i still need to run CI, regression, and add a unit test.