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 tests #62

Merged
merged 2 commits into from
Jan 12, 2022
Merged

Fix tests #62

merged 2 commits into from
Jan 12, 2022

Conversation

michel2323
Copy link
Contributor

I played around with Diffractor and fixed some issues with the tests. Unfortunately, I could not fix some of them.

  • Fixed expression comparison
  • const bwd is now backward mode not forward
  • Marked remaining failing tests as broken with error summary
  • Created Project.toml in test folder per Julia 1.2 Pkg manual

* Fixed expression comparison
* `const bwd` is now backward mode not forward
* Marked remaining failing tests as broken with error summary
* Created Project.toml in `test` folder per Julia 1.2 Pkg manual
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2021

Codecov Report

Merging #62 (0984353) into main (be4eeb5) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   57.04%   57.08%   +0.04%     
==========================================
  Files          21       21              
  Lines        2158     2158              
==========================================
+ Hits         1231     1232       +1     
+ Misses        927      926       -1     
Impacted Files Coverage Δ
src/tangent.jl 60.20% <0.00%> (-5.11%) ⬇️
src/stage1/forward.jl 86.98% <0.00%> (-4.11%) ⬇️
src/stage1/recurse_fwd.jl 94.28% <0.00%> (-2.89%) ⬇️
src/stage1/generated.jl 75.45% <0.00%> (-1.26%) ⬇️
src/stage1/recurse.jl 96.18% <0.00%> (+2.60%) ⬆️
src/extra_rules.jl 50.00% <0.00%> (+2.77%) ⬆️

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 be4eeb5...0984353. Read the comment docs.

@@ -148,7 +150,7 @@ end
@test gradient(x -> sum(abs2, x .+ 1.0), zeros(3))[1] == [2.0, 2.0, 2.0]

const fwd = Diffractor.PrimeDerivativeFwd
const bwd = Diffractor.PrimeDerivativeFwd
const bwd = Diffractor.PrimeDerivativeBack
Copy link
Collaborator

Choose a reason for hiding this comment

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

whoops.

@Keno Keno requested a review from simeonschaub October 29, 2021 21:14
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.

Thanks! Looks good to me.

test/runtests.jl Outdated
Comment on lines 191 to 192
# Higher order control flow not yet supported
@test_broken (x->x^5)'''(1.0) == 60.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Higher order control flow not yet supported
@test_broken (x->x^5)'''(1.0) == 60.
@test (x->(x*x)*(x*x)*x)''' == 60.
# Higher order control flow not yet supported (https://github.com/JuliaDiff/Diffractor.jl/issues/24)
@test_broken (x->x^5)'''(1.0) == 60.

Copy link
Member

Choose a reason for hiding this comment

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

Mind if I commit this and merge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Go for it

Copy link
Member

Choose a reason for hiding this comment

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

It was green before! Now fails at a line outside suggestion box range.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That line tests inference quality. Since CI is running nightly, that might have regressed. I think it's fine to merge anyway, and we'll go back and fix it.

Comment on lines +126 to +127
# Error: ArgumentError: Tangent for the primal Tangent{Tuple{Float64, Float64}, Tuple{Float64, Float64}}
# should be backed by a NamedTuple type, not by Tuple{Tangent{Tuple{Float64, Float64}, Tuple{Float64, Float64}}}.
Copy link
Member

Choose a reason for hiding this comment

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

JuliaDiff/ChainRulesCore.jl#503 should address this, but the integration test still seems to be failing. Probably fine mark these as broken for now, until I figure that out.

@Moelf
Copy link

Moelf commented Dec 7, 2021

should this be merged?

Co-authored-by: Simeon Schaub <[email protected]>
@mcabbott mcabbott merged commit bc22ad6 into JuliaDiff:main Jan 12, 2022
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.

6 participants