Skip to content

Fix tests#62

Merged
mcabbott merged 2 commits intoJuliaDiff:mainfrom
michel2323:ms/fix_tests
Jan 12, 2022
Merged

Fix tests#62
mcabbott merged 2 commits intoJuliaDiff:mainfrom
michel2323:ms/fix_tests

Conversation

@michel2323
Copy link
Contributor

@michel2323 michel2323 commented Oct 27, 2021

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

@michel2323
Fix tests
0984353 
* 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 •
edited
Loading

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.

Keno
Keno reviewed Oct 29, 2021
View reviewed changes
test/runtests.jl

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

@Keno Keno Oct 29, 2021

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
simeonschaub
simeonschaub approved these changes Nov 1, 2021
View reviewed changes
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

@simeonschaub simeonschaub Nov 1, 2021

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

@mcabbott mcabbott Jan 12, 2022

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

@Keno Keno Jan 12, 2022

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

@mcabbott mcabbott Jan 12, 2022

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

@Keno Keno Jan 12, 2022

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.

test/runtests.jl
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

@simeonschaub simeonschaub Nov 1, 2021

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?

@mcabbott @simeonschaub
Update test/runtests.jl
fb84c69 
Co-authored-by: Simeon Schaub <simeondavidschaub99@gmail.com>
@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

Reviewers

@Keno Keno Keno left review comments

@mcabbott mcabbott mcabbott left review comments

@simeonschaub simeonschaub simeonschaub approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@michel2323 @codecov-commenter @Moelf @Keno @simeonschaub @mcabbott