Change target to tables format for Multivariate regressor. #14
Conversation
ayush-1506
changed the title
|
@ablaom I'm not sure why the test fails on travis; they pass locally. Any idea? |
| @@ -65,7 +65,7 @@ function collate(model::Union{NeuralNetworkRegressor, MultivariateNeuralNetworkR | |||
| row_batches = Base.Iterators.partition(1:length(y), batch_size) | |||
|
|
|||
| Xmatrix = MLJModelInterface.matrix(X)' | |||
| if y isa AbstractVector{<:Tuple} | |||
| if y isa Tables.MatrixTable | |||
| return [(Xmatrix[:, b], ymatrix[:, b]) for b in row_batches] | |||
There was a problem hiding this comment.
This should be if Tables.istable(y) not if y isa Tables.MatrixTable, which is too restrictive. For, examlple, user is allowed to send a DataFrame.
src/regressor.jl
Outdated
| data = collate(model, X, y, model.batch_size) | ||
|
|
||
| target_is_multivariate = y isa AbstractVector{<:Tuple} | ||
| target_is_multivariate = y isa Tables.MatrixTable |
|
Sorry about the n_output comment. My mistake. That is weird. Your PR is also passing locally for me. |
|
Is this a clue? It seems travis is picking an ancient version of MLJBase for testing. We are up to 0.12.1 now, which is the version my local test used.
|
|
I'll look into this. Not sure why it's picking up an older version. I could try pinning MLJBase to the required version, hopefully that should solve it. (Although I guess this shouldn't be the preferred method) |
|
FYI: As I understand it, [compat] declarations are ignored for packages not listed under [deps] (see JuliaLang/Pkg.jl#1179) (although this may change in the future (JuliaLang/Pkg.jl#1285)). |
|
@ablaom At the moment, all packages in [deps] are also declared in [compat]. How is that related to MLJBase? |
|
Also, what I'm unable to understand why travis passed on the previous PRs but fails for this. (I've not made any additional changes to Project.toml here, so it should naturally pull the same version of MLJBase (0.12.0) as in previous PRs, right?) |
For new release requests to be fast-tracked (auto-merged into General.jl) it is now a requirement that all [deps] not in the standard library be given [compat] upper bounds. As to your question, " How is that related to MLJBase?" I'm not sure I understand what you mean. Of course the fact that we cannot cap versions of "test-only" packages is a flaw, but this is a bigger issue, as I point out above, and should not concern us here: As I understand it, testing ought to load the latest version of [targets] packages not creating conflicts with the [deps]/[compat] specification. In the case of MLJBase, this should be 0.12.1 (or thereabouts) and I have no idea why travis is not using this version.
Sorry, me neither. Have you tried re-triggering travis with a trivial commit? |
ayush-1506
changed the title
|
Just tried that, still fails. @tlienart would be great if you could also have a look when you get time. Maybe there's something we might have overlooked. Thanks |
|
wrong compat bound for Tables, will fix |
|
Uh I don't have write access... Basically most important one is to bump the tables compat to Notes: (at the risk of sounding annoying)
Also ideally please give Anthony and I write access (I'm assuming Anthony already has it). Otherwise it's great that MLJFlux is back on track! |
|
Make it an explicit dependency, add compat bounds and see what fails on travis |
|
Given @tlienart admin access |
|
@tlienart I tried a bunch of things, nothing seems to work. (Even locally, I deleted the manifest file and did a clean install and it pulls the older MLJBase). Not quite sure what's going wrong. Would be great if you could take a look too. Thanks. |
|
Okay, finally we are getting somewhere. By retroactively adding upper bounds to all versions of all MLJ repos, we get useful conflict error from MLJFlux travis test, suggesting MLJScientifictTypes is the culprit: It needs to update its ColorTypes [compat], which I am now working on. |
Fixes: #11