-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
definitely, we also get nicer prints of the opt_state has a bonus |
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! |
eltype(momentum)
everywhere, ignore typeof(η)
eltype(x)
everywhere, ignore typeof(η)
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.
All rules now changed, and tests pass. The big question is probably whether this counts as a breaking change, or not?
test/runtests.jl
Outdated
@static if VERSION <= v"1.10-" | ||
# In July 2023, Yota works on 1.9 but not nightly (1.11) | ||
using Yota |
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.
This doesn't prevent an error from Yota not compiling on master. Could revert as it's unrelated to this PR.
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.
since this doesn't solve the CI problem let's remove the change from this PR
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.
Reverted.
We removed |
In fact I doubt anyone types that, but the concern is things like BSON, right? |
This reverts commit abf0e13.
I merge this now, we can think about whether anything else wants to be ganged into a breaking release. |
Tagging a new breaking release now since this fixes a lot of problems and I don't see any breaking change on the horizon. |
First commit should fix #150, by working all in Float32.
Chooses to do this by always following the momentum fromchanged tosetup
, not thex
, but perhaps that's messy?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?