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

Readme example does not seem correct #11

Closed
ablaom opened this issue May 10, 2021 · 7 comments
Closed

Readme example does not seem correct #11

ablaom opened this issue May 10, 2021 · 7 comments
Labels
documentation Improvements or additions to documentation

Comments

@ablaom
Copy link

ablaom commented May 10, 2021

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
@ablaom ablaom added the documentation Improvements or additions to documentation label May 10, 2021
@davnn
Copy link
Member

davnn commented May 10, 2021

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
Copy link
Author

ablaom commented May 10, 2021

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
Copy link
Member

davnn commented May 11, 2021

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
Copy link
Author

ablaom commented Oct 5, 2021

@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
Copy link
Member

davnn commented Oct 5, 2021

@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 JuliaAI/MLJ.jl#780.

@davnn
Copy link
Member

davnn commented Nov 11, 2021

The README and documentation is up to date now.

EDIT: @ablaom MLJ docs will follow shortly.

@davnn davnn closed this as completed Nov 11, 2021
@ablaom
Copy link
Author

ablaom commented Nov 12, 2021

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants