OutlierDetectionJL / OutlierDetection.jl

Fast, scalable and flexible Outlier Detection with Julia
https://outlierdetectionjl.github.io/OutlierDetection.jl/dev/
MIT License
80 stars 8 forks source link

Readme example does not seem correct #11

Closed ablaom closed 2 years ago

ablaom commented 3 years ago
julia> model = fit(lof, X[train, :]')
ERROR: BoundsError: attempt to access 6×3772 Matrix{Float64} at index [[1, 2, 3, 4, 5, 6, 7, 8, 9, 10  …  1877, 1878, 1879, 1880, 1881, 1882, 1883, 1884, 1885, 1886], 1:3772]
Stacktrace:
 [1] throw_boundserror(A::Matrix{Float64}, I::Tuple{Vector{Int64}, Base.Slice{Base.OneTo{Int64}}})                                             
   @ Base ./abstractarray.jl:651
 [2] checkbounds
   @ ./abstractarray.jl:616 [inlined]
 [3] _getindex
   @ ./multidimensional.jl:831 [inlined]
 [4] getindex(::Matrix{Float64}, ::Vector{Int64}, ::Function)
   @ Base ./abstractarray.jl:1170
 [5] top-level scope
   @ REPL[71]:1
 [6] top-level scope
   @ ~/.julia/packages/CUDA/k52QH/src/initialization.jl:81

I guess X[train,:]' was meant here?

And score does not appear to be defined:

julia> using OutlierDetection
[ Info: Precompiling OutlierDetection [262411bb-c475-4342-ba9e-03b8c0183ca6]

julia> score
ERROR: UndefVarError: score not defined
Stacktrace:
 [1] top-level scope
   @ :0
 [2] top-level scope
   @ ~/.julia/packages/CUDA/k52QH/src/initialization.jl:81

julia> OutlierDetection.score
ERROR: UndefVarError: score not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:26
 [2] top-level scope
   @ REPL[5]:1
 [3] top-level scope
   @ ~/.julia/packages/CUDA/k52QH/src/initialization.jl:81
davnn commented 3 years ago

I'm sorry, the docs currently only reflect the master branches of the corresponding packages. Installing both OutlierDetection and OutlierDetectionData from master should work, only ODDS.read was changed to ODDS.loadin OutlierDetectionData.

ablaom commented 3 years ago

There's still an issue with fit, unless one also has MLJBase (or MLJ) loaded. The source of this is your new use of MLJModelInterface.matrix. And you have left an adjoint behind, which does not make sense now X is a data frame.

Maybe you want to avoid MLJ altogether in your "local" API. I don't see anything wrong with using matrices there, but if you want to allow generic tables input, then you could use Tables.matrix instead of the MLJModelInterface one, which does not work unless MLJBase is loaded.

Of course, if you don't care to have a separate local API at all, then these comments are not relevant.

davnn commented 3 years ago

Good point, I didn't know that MLJModelInterface.matrix requires MLJ. I changed to the Tables.matrix-conversion for now, but might switch to matrix-only in the future.

ablaom commented 3 years ago

@davnn I wonder if it wouldn't be timely to make some sort of doc update, now that the detector models are MLJ-discoverable and the API has stabilised somewhat. It needn't too comprehensive just now, but correct would be good 😉

davnn commented 3 years ago

@davnn I wonder if it wouldn't be timely to make some sort of doc update, now that the detector models are MLJ-discoverable and the API has stabilised somewhat. It needn't too comprehensive just now, but correct would be good 😉

I'm working on it 👍

Edit: One reason why the docs are not finished yet is that I'm not happy with the API as it is right now, see https://github.com/alan-turing-institute/MLJ.jl/issues/780.

davnn commented 2 years ago

The README and documentation is up to date now.

EDIT: @ablaom MLJ docs will follow shortly.

ablaom commented 2 years ago

Looks great, thanks!