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

Fix computing of array element type for ∇eachslice #808

Merged
merged 24 commits into from
Sep 18, 2024

Conversation

BioTurboNick
Copy link
Contributor

@BioTurboNick BioTurboNick commented Sep 16, 2024

Fixes #807

Added test as well. Two lines really belong in ChainRulesCore, not sure what the process for that is.

@BioTurboNick
Copy link
Contributor Author

Other than the formatter, all the failures seem to be due to the fact that the changes mean Julia currently isn't fully inferring the output anymore. I'll see if there's anything to do about that.

@BioTurboNick
Copy link
Contributor Author

Switching to Base.promote_eltype addresses the inference issue, but that's not a public API in base.

@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented Sep 16, 2024

Huh. I'm not sure why the inference test is failing on 1.6 only. It's inferring Union{NoTangent, Vector{Float64}) instead of just Vector{Float64}.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

I believe the original intent with the design of similar was to support cases like Vector{Vector{Float64}} where zero(Vector{Float64}). That of course doesn't work though since similar will just produce #undefs here making this fail anyways.

Looks good to me minus the methods that need to be moved to ChainRulesCore. Could you make a PR there (ideally with some unit tests), so we can tag a new version there and then change the compat here.

src/rulesets/Base/indexing.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

Just needs to drop the changes already in JuliaDiff/ChainRulesCore.jl#682 and bump the compat for ChainRulesCore

@BioTurboNick
Copy link
Contributor Author

done and done

@simeonschaub
Copy link
Member

Hmm, looks like the format suggestions are not working properly (Maybe because it's on a fork?) If you look at the CI logs, you can see that it complains about the formatting of line 257 in the tests. Once that's fixed this looks good to go!

@devmotion
Copy link
Member

(Maybe because it's on a fork?)

Yes, due to restricted permissions for forks the suggestions can only be shown in the logs but not as actual suggestions in the PR.

@simeonschaub simeonschaub merged commit e245d50 into JuliaDiff:main Sep 18, 2024
8 of 10 checks passed
@simeonschaub
Copy link
Member

Thanks for the awesome contribution!

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.

∇eachslice causes UndefRefError when input array contains references
3 participants