Skip to content
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

Depend on Optimisers.jl #1864

Merged
merged 2 commits into from
Mar 5, 2022
Merged

Depend on Optimisers.jl #1864

merged 2 commits into from
Mar 5, 2022

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Feb 5, 2022

This is I think the minimal step to make Flux usable with Optimisers.jl: Both should see the same function trainable.

Will fail until Optimisers.jl 0.2 is tagged; 0.1 does not have this function.

If we aren't deleting implicit parameters from v0.13, can we do any more than this? Maybe we can allow setup(opt, ::Params), and write an update! which uses that. But this PR does not do it.

Xref #1481, which completely replaces Flux's code, and I think

@mcabbott mcabbott added this to the v0.13 milestone Feb 5, 2022
@@ -1,12 +1,14 @@
module Flux

# Zero Flux Given
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd totally forgotten about this line 😆

Copy link
Contributor

@cossio cossio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to replace all those using to import, but could be done in another PR

src/Flux.jl Outdated
@reexport using NNlib

using Zygote
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this can go away (given the line just below)?. Or at least replaced by an import.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually to remove using Zygote one needs to replace also Flux.withgradient with Zygote.withgradient in the recurrent.jl tests for tests to pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be better served by a separate PR that does some general import cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree.

@mcabbott
Copy link
Member Author

mcabbott commented Mar 5, 2022

Optimisers.jl 0.2 is now tagged.

@mcabbott mcabbott requested a review from ToucheSir March 5, 2022 04:51
@mcabbott mcabbott merged commit 5e05de7 into FluxML:master Mar 5, 2022
@mcabbott mcabbott deleted the optimisers branch March 5, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants