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

Add Duplicated methods #192

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Add Duplicated methods #192

merged 6 commits into from
Nov 8, 2024

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented Nov 8, 2024

This moves part of FluxML/Flux.jl#2471 to an extension here.

Bumps Julia support to 1.10.

PR Checklist

  • Tests are added
  • Documentation, if applicable

Comment on lines +552 to +555
shared .= 1
Optimisers.update!(st2, model, grad)
model.x ≈ [0.8] # This is wrong, but don't make it a test.
# Ideally, perhaps the 3-arg update! could notice that grad.x===grad.y, and not accumulate the gradient in this case?
Copy link
Member

Choose a reason for hiding this comment

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

I get what you mean, but do we promise to handle shared gradient arrays? Might be easier to add a warning in the docs. Maybe a broken test outside of the Enzyme test suite if we really care about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

At present (after this PR) I think the usual path with Zygote and this new path with Enzyme should always agree.

Whether the normal path can auto-detect this (as in the comment) I'm not really sure. It's possible to make Zygote return two components which are ===, and it's conceivable that this could happen with shared parameters.

Nevertheless, detecting this would probably be better than not. In real use, e.g. you use the same layer twice, Zygote is certainly going to return two new arrays for shared parameters.

@mcabbott mcabbott merged commit 2639523 into FluxML:master Nov 8, 2024
3 of 4 checks passed
@mcabbott mcabbott deleted the enzyme1 branch November 8, 2024 21:08
mashu pushed a commit to mashu/Optimisers.jl that referenced this pull request Nov 14, 2024
* add Duplicated methods

* add test

* test for shared params + minimal docs

* remove 1.6 CI

* indent by two spaces

* fix doctest
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