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

Use eltype(x) everywhere, ignore typeof(η) #151

Merged
merged 19 commits into from
Aug 21, 2023
Merged

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Jul 26, 2023

First commit should fix #150, by working all in Float32. Chooses to do this by always following the momentum from setup, not the x, but perhaps that's messy? changed to eltype(x) now.

Since the type of η is now ignored, perhaps we should just remove all the type parameters? Then e.g. Adam(0) == Adam(0.0), fixes #119, closes #120. Also closes #86 (redundant).

Not all rules are changed yet, RFC?

@CarloLucibello
Copy link
Member

we should just remove all the type parameters?

definitely, we also get nicer prints of the opt_state has a bonus

@ToucheSir
Copy link
Member

If using the eltype of momentum is too restrictive, we could always change to some promoted eltype of various parts of input + state (e.g. momentum + param + gradient eltypes). Otherwise 👍 from me though, nice to see that this works!

src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
@mcabbott mcabbott changed the title Use eltype(momentum) everywhere, ignore typeof(η) Use eltype(x) everywhere, ignore typeof(η) Jul 26, 2023
Copy link
Member Author

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

All rules now changed, and tests pass. The big question is probably whether this counts as a breaking change, or not?

Project.toml Outdated Show resolved Hide resolved
test/runtests.jl Outdated
Comment on lines 41 to 43
@static if VERSION <= v"1.10-"
# In July 2023, Yota works on 1.9 but not nightly (1.11)
using Yota
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't prevent an error from Yota not compiling on master. Could revert as it's unrelated to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

since this doesn't solve the CI problem let's remove the change from this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted.

src/rules.jl Show resolved Hide resolved
@darsnack
Copy link
Member

We removed Descent{T}(…) type constructors, so unfortunately, yes.

@mcabbott
Copy link
Member Author

In fact Descent{T} was the one I left, since IIRC someone had a paper doing "gradient descent" with complex learning rate.

I doubt anyone types that, but the concern is things like BSON, right?

src/rules.jl Show resolved Hide resolved
src/rules.jl Show resolved Hide resolved
src/rules.jl Show resolved Hide resolved
@mcabbott
Copy link
Member Author

I merge this now, we can think about whether anything else wants to be ganged into a breaking release.

@mcabbott mcabbott merged commit 5ac2d6f into FluxML:master Aug 21, 2023
3 of 4 checks passed
@mcabbott mcabbott deleted the types2 branch August 21, 2023 13:52
@CarloLucibello
Copy link
Member

Tagging a new breaking release now since this fixes a lot of problems and I don't see any breaking change on the horizon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in update! for Metal arrays and Adam optimiser Adam(0) fails
4 participants