search

FluxML / Optimisers.jl

Optimisers.jl defines many standard optimisers and utilities for learning loops.
https://fluxml.ai/Optimisers.jl
MIT License
74 stars 22 forks source link

Change `update!` to `update!!` #116

Open mcabbott opened 1 year ago

mcabbott commented 1 year ago

This function makes a good-faith effort to mutate, but provides no guarantee. That's unlike freeze!, and unlike Base functions, which guarantee they will mutate. There's some convention that !! means this, e.g. from https://github.com/JuliaFolds/BangBang.jl, so this PR renames it.

Doing so opens the door to having a new update! in v0.3 which does guarantee to mutate, else fails with an error.

That would be nice for Flux, where otherwise there's an awkward transition: v0.13's Flux.Optimise assumes everything mutates. If you upgrade to Optimisers.jl's update!, and discard what it returns, your model will work fine right now as all old models are mutable. But it will also now silently ignore and new immutable parameters. We can't check that you didn't discard the output.

Doing this also opens the door to simplifying the interface. There is no need for update! to return the state tree, since this is sure to be mutated. And it's confusing to return two things & have to know the order. So perhaps it should return only the model? Or will it be too confusing compared to update which must return both?

The must-mutate update! could return nothing, to emphasise its difference. Code which goes model, tree = update!(...) will fail loudly and clearly with nothing. It may fail in very strange ways if this returns only the model. That's a reason to have 0 or 2 returns, never 1.

ToucheSir commented 1 year ago

Backwards compat aside, is there a need for update! if we have update!!? Just to throw an error if immutable params are encountered, I guess?