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

Remove Deprecations. Replace tangent_transforms functionality #180

Merged
merged 12 commits into from
Jul 23, 2021
Merged

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Jun 18, 2021

Closes #113, closes #164.

Do we also want #179?

Make sure thunks are supported in rules in

@oxinabox
Copy link
Member

Do we also want #179?

No, probably not.

Why are we releasing this right now?
Or are we just getting ready with the PR open?

@mzgubic
Copy link
Member Author

mzgubic commented Jun 18, 2021

Yeah, just making sure we don't forget to deprecate things this time. We are planning to release two breaking versions though, right? - One which requires rules to support thunks (0.8) and one which requires the support for ZeroTangent as well (1.0)?

@oxinabox
Copy link
Member

We are planning to release two breaking versions though, right? - One which requires rules to support thunks (0.8) and one which requires the support for ZeroTangent as well (1.0)?

I was thinking just 1.

oxinabox pushed a commit that referenced this pull request Jul 21, 2021
@oxinabox oxinabox changed the title Release ChainRulesTestUtils 0.8 Remove Deprecations Jul 22, 2021
@oxinabox
Copy link
Member

looks like we have some issues with it calling Float64(::Thunk)
I will debug this later today.

@oxinabox
Copy link
Member

This is now passing tests.
Because I added the missing unthunks.
Many of them were for frules.
I don't think using thunks in frules is a good idea because it breaks mutation support.

I think we should stop testing thunks in frules.

@mzgubic
Copy link
Member Author

mzgubic commented Jul 23, 2021

Very naive q, but how does it break mutation support? Is it that frules which support mutation can't receive thunks?

@oxinabox
Copy link
Member

Very naive q, but how does it break mutation support? Is it that frules which support mutation can't receive thunks?

frules for mutating primal functions can't recieve thunks.
The first rule of mutation support club, is that mutation of the primal needs to result in matching mutation of the tangent.
Thunks are effectively immutable.
In general, for this reason, mutation support is much more fussy about the types involved.

Shall i remove that from this PR?
Probably easiest as I can see more clearly which frules i changed that i should not have

@mzgubic
Copy link
Member Author

mzgubic commented Jul 23, 2021

Thanks, makes sense, I was just wondering whether there was anything else to it.

I assume mutation support is more important, right? So let's remove the default? I can also do that

@oxinabox
Copy link
Member

I assume mutation support is more important, right? So let's remove the default? I can also do that

I will do this

test/testers.jl Outdated Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
@oxinabox oxinabox changed the title Remove Deprecations Remove Deprecations. Replace tangent_transforms functionality Jul 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #180 (83c01ee) into master (492364c) will increase coverage by 3.13%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   87.80%   90.94%   +3.13%     
==========================================
  Files          12       11       -1     
  Lines         328      287      -41     
==========================================
- Hits          288      261      -27     
+ Misses         40       26      -14     
Impacted Files Coverage Δ
src/ChainRulesTestUtils.jl 100.00% <ø> (ø)
src/global_config.jl 66.66% <0.00%> (+16.66%) ⬆️
src/check_result.jl 89.06% <100.00%> (ø)
src/testers.jl 92.85% <100.00%> (-0.84%) ⬇️

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 492364c...83c01ee. Read the comment docs.

@mzgubic
Copy link
Member Author

mzgubic commented Jul 23, 2021

should we test that we catch bad rules with this?

docs/src/index.md Show resolved Hide resolved
Comment on lines 102 to +103
Test Summary: | Pass Total
test_scalar: relu at 0.5 | 10 10
test_scalar: relu at 0.5 | 11 11

Choose a reason for hiding this comment

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

[Documenter (fix doctests)] reported by reviewdog 🐶

Suggested change
Test Summary: | Pass Total
test_scalar: relu at 0.5 | 10 10
test_scalar: relu at 0.5 | 11 11

docs/src/index.md Show resolved Hide resolved
@mzgubic
Copy link
Member Author

mzgubic commented Jul 23, 2021

I approve the commits added (but can't click approve). One thought - should we also keep the empty deprecated.jl file in the tests so that we don't forget to add it?

@oxinabox
Copy link
Member

One thought - should we also keep the empty deprecated.jl file in the tests so that we don't forget to add it?

Maybe, I think it is not a big deal either way, So I am not going to retrigger CI for that

@oxinabox oxinabox merged commit 6df5395 into master Jul 23, 2021
@oxinabox oxinabox deleted the mz/0.8 branch July 23, 2021 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants