-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
use MLUtils #1874
use MLUtils #1874
Conversation
Merge? |
mergity merge? |
I guess the main issue here is that the new Otherwise this is a drop-in replacement, so good enough for me. |
Other question is what happens when MLUtils -> MLDataUtils? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more concerns on my end
There are many warnings like this in the tests:
No rush but it would be nice to update all of them. |
Can we have a release with the nicer DataLoader? |
These are the blockers: https://github.com/FluxML/Flux.jl/milestone/3 |
This is replacing some utility functions and the
DataLoader
with their equivalent versions in MLUtils.jl.On the DataLoader inferrability
After this, the
DataLoader
will accept generic datasets exposing theMLUtils.numobs
andMLUtils.getobs
interface (something that we should probably document somewhere). This has been discussed in #1282.Unfortunately we lose inferrability, e. g.
@inferred first(loader)
will fail. This is because the previous implementation was just wrong for generic datasets. It waswhere
D
is the underlying data type. So this PR will reopen #1159. This is an example of the current problematic behavior on master:TODO:
Close #1227