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

Change update! to update!! #116

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

mcabbott
Copy link
Member

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
Copy link
Member

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?

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.

2 participants