Give DataLoader a proper eltype - #253
Merged
Merged
Conversation
`eltype(::DataLoader)` returned `Any` because no `eltype` method was
defined (the previous attempt was commented out — it rebuilt a
`BatchView` *without* the loader's `collate`, producing an eltype
inconsistent with the actual batches and breaking `collect`).
A `DataLoader` iterates the batches of its wrapped `BatchView`/`ObsView`
(`d._data`), so its eltype is exactly `eltype(d._data)`, computed from
the real `collate`. Define `eltype(::Type{<:DataLoader{T}}) = eltype(T)`
(and make `BatchView`'s `eltype` type-based so it composes).
`IteratorEltype` is kept as `EltypeUnknown`: for containers whose
per-batch `getobs` is not fully inferable (e.g. a `Dict` with
heterogeneous value types) the declared eltype is only a supertype of
the real batches, so `collect` is left to narrow the element type from
the batches rather than pre-committing to the wider type. This keeps
`collect` behaviour (and its tests) unchanged while fixing `eltype`.
Un-breaks the `eltype(DataLoader)` cases in test/dataloader.jl for
arrays, tuples, named tuples and homogeneous dicts. The heterogeneous
`Dict` case stays `@test_broken`: it needs `getobs(::Dict)` to be
inferable (#227 item B, lives in MLCore).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
eltype(DataLoader)items of #227 (section A).Problem
eltype(::DataLoader)returnedAny— noeltypemethod was defined. The previous attempt (commented out in the source) rebuilt aBatchViewwithout the loader'scollate, so it computed an eltype inconsistent with the actual batches and brokecollect.Change
A
DataLoaderiterates the batches of its wrappedBatchView/ObsView(d._data), which is built with the loader's realcollate. So its element type is exactlyeltype(d._data):BatchView'seltypeis made type-based so it composes with the above.IteratorEltypeis deliberately kept asEltypeUnknown(): for containers whose per-batchgetobsis not fully inferable (aDictwith heterogeneous value types), the declared eltype is only a supertype of the real batches. KeepingEltypeUnknownletscollectnarrow the element type from the batches themselves — socollect's behaviour (and its existing tests) are unchanged, whileeltype(d)now returns a useful type.Tests
Un-breaks the
eltype(DataLoader)cases intest/dataloader.jlfor arrays, tuples, named tuples, and homogeneous dicts (changed@test_broken→@test). The fulldataloader(394 pass, 1 broken) andbatchview(585 pass) testsets pass locally.The one remaining
@test_brokenis the heterogeneousDictcase (Matrix+Vectorvalues): its eltype needsgetobs(::Dict)to be type-inferred, which is #227 item B and lives in MLCore.jl. This PR does not close #227 (a tracking issue).🤖 Generated with Claude Code