Sorting out issue with calling predict on matrix

[pat-alt]

Closes #218

[ablaom]

@pat-alt Thanks for looking at this.

Yes, I think your proposal ought to work: matrices can be provided not only to predict, but also to fit, which we should have for consistency.

What's left to do is:

• Add tests
• Update the scitype declarations
• Update the docstrings

For example, this code should be changed to

MLJModelInterface.metadata_model(NeuralNetworkClassifier,
                                 input=Union{AbstractMatrix{Continuous}, Table(Continuous)},
                                 target=AbstractVector{<:Finite},
                                 path="MLJFlux.NeuralNetworkClassifier")

Similar changes can be made at regressor.jl

[pat-alt]

I've updated the scitype declarations for regression (here and here) and for classification (here). For testing, I thought I'd just rerun basic tests with a matrix input (see here). Unfortunately, I can't immediately make sense of the error I'm getting.

[ablaom]

Thanks for persisting with this.

This error usually means we're trying to grab the schema of an object that doesn't have one. I believe the culprit is here. We could add a second method dispatching on matrices to address. There are similar issues for shape in regressor.jl.

[pat-alt]

Thanks! I believe that's fixed now, though I did still get some warnings like this one:

Warning: The number and/or types of data arguments do not match what the specified model
│ supports. Suppress this type check by specifying `scitype_check_level=0`.
│ 
│ Run `@doc MLJFlux.NeuralNetworkRegressor` to learn more about your model's requirements.
│ 
│ Commonly, but non exclusively, supervised models are constructed using the syntax
│ `machine(model, X, y)` or `machine(model, X, y, w)` while most other models are
│ constructed with `machine(model, X)`.  Here `X` are features, `y` a target, and `w`
│ sample or class weights.
│ 
│ In general, data in `machine(model, data...)` is expected to satisfy
│ 
│     scitype(data) <: MLJ.fit_data_scitype(model)
│ 
│ In the present case:
│ 
│ scitype(data) = Tuple{Table{AbstractVector{Continuous}}, AbstractVector{Continuous}}
│ 
│ fit_data_scitype(model) = Tuple{Union{Table{<:AbstractVector{<:Continuous}}, AbstractMatrix{Continuous}}, AbstractVector{<:Finite}}

Edit: As for docstrings, I've updated for the classifier and regressors as follows:

- `X` is either a `Matrix` or any table of input features (eg, a `DataFrame`) whose columns are of scitype
  `Continuous`; check column scitypes with `schema(X)`. If `X` is a `Matrix`, it is assumed to have columns corresponding to features and rows corresponding to observations.

[ablaom]

Thanks! I believe that's fixed now, though I did still get some warnings like this one:

According to the warning you are providing the regressor with a Continuous target, rather than a Finite one (categorical vector).

[ablaom]

Sorry, rather the scitype declaration for the regressor is wrong. You need Continuous in the target_scitype where you appear to have Finite.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.14 🎉

Comparison is base (452c09d) 92.73% compared to head (49d11af) 92.88%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #219      +/-   ##
==========================================
+ Coverage   92.73%   92.88%   +0.14%     
==========================================
  Files          11       11              
  Lines         303      309       +6     
==========================================
+ Hits          281      287       +6     
  Misses         22       22              

Impacted Files	Coverage Δ
src/classifier.jl	100.00% <100.00%> (ø)
src/core.jl	94.87% <100.00%> (+0.13%)	⬆️
src/regressor.jl	100.00% <100.00%> (ø)
src/types.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pat-alt]

Sorry, rather the scitype declaration for the regressor is wrong. You need Continuous in the target_scitype where you appear to have Finite.

You're right, thanks. Fixed that now, but I still get some broken tests (e.g. here. Is this expected?

[ablaom]

Fixed that now, but I still get some broken tests (e.g. here. Is this expected?

Yes. Some of the broken tests are because you are looking at CPU tests, where some tests are excluded and get marked as broken.

The remaining broken tests are explained here #87 .

So no further action on your part needed here. I will finish my review shortly, thanks.

[ablaom]

I'm happy with this as is. Many thanks.

However, perhaps you would consider one additional enhancement, for consistency:
The MulitargetNeuralNetworkRegressor accepts the target as a table, but not as a matrix. It seems this would not be difficult to fix (but I haven't checked):

• In fitresult(model::MultitargetNeuralNetworkRegressor, chain, y) method return nothing instead of Table.schema(y).names if y is a matrix.
• In shape make the change for treating y you already have made for X.
• In predict return a matrix if target_column_names is nothing.

If you'd rather proceed as we are, then I'll just open an new issue and reference this comment.

[pat-alt]

Thanks, good idea! I've made the necessary adjustments and updated tests accordingly. Had to make one additional small modification to the collate function. I've also updated the docstring.

[ablaom]

I'm trying to understand why this fix was needed. The nrows function is supposed to work for matrices as well as tables:

using MLJBase
julia> y = rand(2, 6)
julia> nrows(y)
2

julia> nrows(y')
6

And line 230 below should already take care of the conversion of y to a matrix, no?

[pat-alt]

This was giving me an error previously, because the nrows method defined in the same file expects a table (or else it throws an ArgumentError. I've adjusted that and tests are passing.

[ablaom]

Oh, I see. We're not using nrows from MLJBase.jl - I forgot. Thanks for humouring me with the explanation!

[ablaom]

Looks good to go.