Skip to content

Revert "Rule for mean(f,x)"#619

Merged
rofinn merged 2 commits intomainfrom
revert-615-mean_f_x
May 18, 2022
Merged

Revert "Rule for mean(f,x)"#619
rofinn merged 2 commits intomainfrom
revert-615-mean_f_x

Conversation

@oxinabox
Copy link
Member

@oxinabox oxinabox commented May 17, 2022

Reverts #615

because
This broke Invenia's code base. Something went wrong with how this works with NamedDims.jl.
I am reverting this til it can be debugged

2022-05-17T18:24:11Z	MethodError: no method matching iterate(::Nothing)
2022-05-17T18:24:11Z	Stacktrace:
2022-05-17T18:24:11Z	  [1] indexed_iterate(I::Nothing, i::Int64)
2022-05-17T18:24:11Z	    @ Base ./tuple.jl:89
2022-05-17T18:24:11Z	  [2] rrule(config::Zygote.ZygoteRuleConfig{Zygote.Context}, ::typeof(Statistics.mean), f::NamedDims.NamedDimsArray{(:target, :node), Float32, 2, Matrix{Float32}}, x::StatsBase.AnalyticWeights{Float32, Float32, Vector{Float32}}; dims::Function)
2022-05-17T18:24:11Z	    @ ChainRules ~/.julia/packages/ChainRules/OJ3vT/src/rulesets/Statistics/statistics.jl:26
2022-05-17T18:24:11Z	  [3] rrule(config::Zygote.ZygoteRuleConfig{Zygote.Context}, ::typeof(Statistics.mean), f::NamedDims.NamedDimsArray{(:target, :node), Float32, 2, Matrix{Float32}}, x::StatsBase.AnalyticWeights{Float32, Float32, Vector{Float32}})
2022-05-17T18:24:11Z	    @ ChainRules ~/.julia/packages/ChainRules/OJ3vT/src/rulesets/Statistics/statistics.jl:26
2022-05-17T18:24:11Z	  [4] chain_rrule
2022-05-17T18:24:11Z	    @ ~/.julia/packages/Zygote/DkIUK/src/compiler/chainrules.jl:217 [inlined]
2022-05-17T18:24:11Z	  [5] macro expansion
2022-05-17T18:24:11Z	    @ ~/.julia/packages/Zygote/DkIUK/src/compiler/interface2.jl:0 [inlined]

Reopens #85, reopens FluxML/Zygote.jl#1128

@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label May 17, 2022
@github-actions github-actions bot removed the needs version bump Version needs to be incremented or set to -DEV in Project.toml label May 17, 2022
@rofinn
Copy link
Contributor

rofinn commented May 17, 2022

Assuming all other test besides Diffractor.jl I'll merge this.

NOTE: The Diffractor.jl test is being disabled in #620 anyway.

@mcabbott
Copy link
Member

mcabbott commented May 17, 2022

This is like #522, there's a StatsBase signature with two arrays. Slightly pirate, since std. lib. does allow that, although it doesn't work:

julia> using Statistics

julia> mean([1,2], [3,4])
ERROR: MethodError: objects of type Vector{Int64} are not callable

Might be almost as quick to copy the opt_out fix used there, as to revert.

@rofinn
Copy link
Contributor

rofinn commented May 18, 2022

Hmm, I think merging the revert seems safer to me for now. I don't think the nightly CI errors should block this, but maybe @mcabbott thinks this shouldn't go in if the fix is sufficiently simple?

@oxinabox
Copy link
Member Author

Reverting the revert is also fast.

In general I am strongly in favor of reverting things that cause problems sooner rather than any delay as people start adding work arounds for them and that makes things more disruptive.

Let's do this and then we do not have to rush to get the final solution in.

@rofinn rofinn merged commit d246d12 into main May 18, 2022
@rofinn rofinn deleted the revert-615-mean_f_x branch May 18, 2022 04:57
@mcabbott
Copy link
Member

mcabbott commented Jul 14, 2022

2 month bump?

I think the fix is something like this:

https://github.com/JuliaDiff/ChainRules.jl/blob/main/src/rulesets/Base/mapreduce.jl#L126-L135

Also, FluxML/Zygote.jl#836 is probably the original issue for this.

@mcabbott
Copy link
Member

mcabbott commented Dec 3, 2022

Bump, after 6 months?

See this is the problem with reverting. While everyone was interested, we could have fixed the weird (not-quite-kosher) StatsBase signature in like a day, back in May. But instead, nobody has quite got around to it in half a year.

@oxinabox
Copy link
Member Author

oxinabox commented Dec 9, 2022

I mean yes, but everyone was also too busy. As you can guess from my current burst of activity I am finally having time to work on this stuff again.
Still yes lets look at it, create a reproducer, and fix this.
I will do so presently

oxinabox added a commit that referenced this pull request Dec 9, 2022
@oxinabox oxinabox mentioned this pull request Dec 9, 2022
3 tasks
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.

3 participants