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 prod #385

Closed
wants to merge 1 commit into from
Closed

add prod #385

wants to merge 1 commit into from

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Mar 9, 2021

I wanted to see if it was as simple as i thought it would be.
The answer is yes.

@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Mar 9, 2021
@codecov-io
Copy link

codecov-io commented Mar 9, 2021

Codecov Report

Merging #385 (912663f) into master (76ef95c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #385   +/-   ##
=======================================
  Coverage   97.80%   97.80%           
=======================================
  Files          20       20           
  Lines        1547     1552    +5     
=======================================
+ Hits         1513     1518    +5     
  Misses         34       34           
Impacted Files Coverage Δ
src/rulesets/Base/mapreduce.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76ef95c...912663f. Read the comment docs.

Comment on lines +25 to +28
test_rrule(prod, randn(5))
test_rrule(prod, randn(5, 6))
test_rrule(prod, randn(5, 6); fkwargs=(;dims=2))
test_rrule(prod, randn(5, 6); fkwargs=(;dims=1))
Copy link
Member

Choose a reason for hiding this comment

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

Needs complex tests as well

#### prod
####

function rrule(::typeof(prod), x::AbstractArray{T}; dims=:) where {T<:CommutativeMulNumber}
Copy link
Member

@willtebbutt willtebbutt Mar 9, 2021

Choose a reason for hiding this comment

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

Would you consider restricting the type of x to a subtype of AbstractArrays?

Copy link
Member Author

@oxinabox oxinabox Mar 9, 2021

Choose a reason for hiding this comment

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

I just matched sum
I am happy to restrict to whatever

Copy link
Member

Choose a reason for hiding this comment

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

Given that we've not tied this down, I propose StridedArray.

@sethaxen
Copy link
Member

sethaxen commented Mar 9, 2021

Can you comment on how this is relates to #335? Does this cover the same cases?

@oxinabox
Copy link
Member Author

oxinabox commented Mar 9, 2021

I was not aware of #335
This is definitely simpler that #335.
So I guess I am missing something

@oxinabox
Copy link
Member Author

oxinabox commented Mar 9, 2021

Yep the following fails:

julia>         test_rrule(prod, [2.0, 0.0, 1.0, 3.5])
test_rrule: prod at ([2.0, 0.0, 1.0, 3.5],): Test Failed at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/AIKXr/src/check_result.jl:19
  Expression: isapprox(actual, expected; kwargs...)
   Evaluated: isapprox([0.0, NaN, 0.0, 0.0], [0.0, 0.8173013508577128, 0.0, 0.0]; rtol = 1.0e-9, atol = 1.0e-9)
Stacktrace:
 [1] check_equal(::Array{Float64,1}, ::Array{Float64,1}; kwargs::Base.Iterators.Pairs{Symbol,Float64,Tuple{Symbol,Symbol},NamedTuple{(:rtol, :atol),Tuple{Float64,Float64}}}) at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/AIKXr/src/check_result.jl:19
 [2] macro expansion at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/AIKXr/src/testers.jl:195 [inlined]
 [3] macro expansion at /usr/local/src/julia/julia-1.5/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115 [inlined]
 [4] test_rrule(::typeof(prod), ::Array{Float64,1}; output_tangent::ChainRulesTestUtils.Auto, fdm::FiniteDifferences.AdaptedFiniteDifferenceMethod{5,1,FiniteDifferences.UnadaptedFiniteDifferenceMethod{7,5}}, check_inferred::Bool, fkwargs::NamedTuple{(),Tuple{}}, rtol::Float64, atol::Float64, kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/AIKXr/src/testers.jl:160

Closing this infavor of #335

@oxinabox oxinabox closed this Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing rule needs version bump Version needs to be incremented or set to -DEV in Project.toml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants