FluxML / MLJFlux.jl

Wrapping deep learning models from the package Flux.jl for use in the MLJ.jl toolbox
http://fluxml.ai/MLJFlux.jl/
MIT License
143 stars 17 forks source link

Off-by-one error in clean! method #227

Closed MarkArdman closed 1 year ago

MarkArdman commented 1 year ago

Bug description

When a model is constructed with batch_size=0 it passes the clean! method without warning and batch_size remains at value 0. However the fit! method attempts to collate the data into batches which is not possible due to the Julia partition method (please find the method below) called in collate throwing an error when attempting to set n as 0.

function partition(c, n::Integer)
    n < 1 && throw(ArgumentError("cannot create partitions of length $n"))
    return PartitionIterator(c, Int(n))
end

I am not entirely certain if 0 is meant to stand for no batching or if un-batched training is not an intended feature, but currently I am assuming it is the latter due to the way training is done, with un-batched training basically not being possible. Therefore I assume this is an off-by-one error and have made the clean! method reject batch_size=0 along with everything below that.

PR Checklist

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.34 :tada:

Comparison is base (bf410e1) 92.92% compared to head (97897ae) 93.26%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #227 +/- ## ========================================== + Coverage 92.92% 93.26% +0.34% ========================================== Files 11 12 +1 Lines 311 312 +1 ========================================== + Hits 289 291 +2 + Misses 22 21 -1 ``` | [Impacted Files](https://app.codecov.io/gh/FluxML/MLJFlux.jl/pull/227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux) | Coverage Δ | | |---|---|---| | [src/mlj\_model\_interface.jl](https://app.codecov.io/gh/FluxML/MLJFlux.jl/pull/227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL21sal9tb2RlbF9pbnRlcmZhY2Uuamw=) | `94.20% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/FluxML/MLJFlux.jl/pull/227/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux)

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