-
Notifications
You must be signed in to change notification settings - Fork 64
Don't use rules for UniformScaling on 1.0 #542
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
Conversation
| ProjectTo(x::UniformScaling) = ProjectTo{UniformScaling}(; λ=ProjectTo(x.λ)) | ||
| ProjectTo(x::UniformScaling{Bool}) = ProjectTo(false) | ||
| (pr::ProjectTo{UniformScaling})(dx::UniformScaling) = UniformScaling(pr.λ(dx.λ)) | ||
| (pr::ProjectTo{UniformScaling})(dx::Tangent{<:UniformScaling}) = UniformScaling(pr.λ(dx.λ)) |
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.
[JuliaFormatter] reported by reviewdog 🐶
| (pr::ProjectTo{UniformScaling})(dx::Tangent{<:UniformScaling}) = UniformScaling(pr.λ(dx.λ)) | |
| function (pr::ProjectTo{UniformScaling})(dx::Tangent{<:UniformScaling}) | |
| return UniformScaling(pr.λ(dx.λ)) | |
| end |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #542 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 15 15
Lines 866 866
=======================================
Hits 809 809
Misses 57 57
Continue to review full report at Codecov.
|
Why is it harder? Can we not just register a tag from a branch? If we merge this PR, and then bump the compat to 1.6 we have to at that point undo the changes this PR introduced. |
| (pr::ProjectTo{UniformScaling})(dx::Tangent{<:UniformScaling}) = UniformScaling(pr.λ(dx.λ)) | ||
| if VERSION >= v"1.6" | ||
| # UniformScaling can represent its own cotangent | ||
| # but shouldn't on Julia 1.0, as rules in CR.jl were added only after mioving to 1.6 |
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.
| # but shouldn't on Julia 1.0, as rules in CR.jl were added only after mioving to 1.6 | |
| # but shouldn't on Julia 1.0, as rules in CR.jl were added only after moving to 1.6 |
|
@mzgubic do you think we should just go and backport the missing rules into CR and tag a backported release? |
|
Yes, I can do that. Let's merge #544 and I will backport JuliaDiff/ChainRules.jl#571 to ChainRules 1.21, as it was before https://github.com/JuliaDiff/ChainRules.jl/pull/577/files |
#533 enforces that
Iis its own tangent. Since this package still allows Julia 1.0, and the current ChainRules does not, this apparently breaks DistributionsAD on Julia 1.5.This PR is one way to avoid that. It would need to be in one release before dropping 1.0 forever.
However, since there has been exactly one release with #533 in it (list https://github.com/JuliaDiff/ChainRulesCore.jl/releases/) the alternative would just be to edit one line in the registry to make that release 1.6-only. And make all new releases here 1.6 only too, which we should probably do in any case.
You could also mess with releases on multiple branches, but that sounds harder.