rename differentials #353

mzgubic · 2021-05-13T16:37:27Z

Closes #305

I've only renamed the definitions here so we can finalise our discussion on what to call different things.

Once we agree, I'll make the rest of the changes.

Edit:

The final list is:

AbstractDifferential -> AbstractTangent
Composite{T} -> Tangent{T}
Zero -> ZeroTangent
DoesNotExist -> NoTangent

And I've left One for another PR since it was causing CI to fail on Julia 1.0 and Julia 1.5

mzgubic · 2021-05-13T16:39:16Z

src/differentials/thunks.jl

@@ -112,7 +112,7 @@ but it should do this more efficently than simply doing this directly.
 Most operations on an `InplaceableThunk` treat it just like a normal `Thunk`;
 and destroy its inplacability.
 """
-struct InplaceableThunk{T<:Thunk, F} <: AbstractThunk
+struct InplaceableThunkedTangent{T<:Thunk, F} <: AbstractThunk


This seems too long and I've changed it only to highlight the inconsistency we get if we keep it as InplaceableThunk.

Maybe we shouldn't use the Tangent suffix for Thunks?

How about InplaceableTangent?

Two reasons against that:

hides arguably the more important info for handling this object (that it is a thunk)

many tangents are "inplaceable" without being this type (so having something named InplaceableTangent is confusing)

Personally I prefer both the current name and the more verbose name

Ok, I've changed it back to the current name. Are there any other open questions or are we happy with this? I'll go ahead and open follow up PRs everywhere

mzgubic · 2021-05-13T16:39:43Z

src/differentials/one.jl

@@ -3,7 +3,7 @@
 The Differential which is the multiplicative identity.
 Basically, this represents `1`.
 """
-struct One <: AbstractDifferential end
+struct OneTangent <: AbstractTangent end


Do we want this to be consistent with ZeroTangent? It seems to be used very rarely anyway.

I think we want to delete this. #354
Can we try deprecating it in favour of true and see if that breaks anything on our reverse dependency tests?

@deprecate One() true

oxinabox · 2021-05-13T17:12:55Z

We need to use @deprecate_binding to deprecate these.
@deprecate_binding is the best form of deprecation.

It should be all
@deprecate_binding Zero ZeroTangent

nickrobinson251 · 2021-05-13T19:01:57Z

src/differentials/abstract_zero.jl

@@ -71,7 +71,7 @@ arguments.
    end
 ```
 """
-struct DoesNotExist <: AbstractZero end
+struct NoPossibleTangent <: AbstractZero end


Shorter and means the same thing?

Suggested change

struct NoPossibleTangent <: AbstractZero end

struct NoTangent <: AbstractZero end

Arguably means a different thing, but might still be good

mzgubic · 2021-05-14T09:40:26Z

@simeonschaub @devmotion @sethaxen @mcabbott thoughts/opinions?

simeonschaub

I didn't mind the old names too much, but I am fine with adopting these new names.

codecov-commenter · 2021-05-17T16:55:56Z

Codecov Report

Merging #353 (cf4d39a) into master (b98e624) will not change coverage.
The diff coverage is 97.74%.

@@           Coverage Diff           @@
##           master     #353   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files          14       14           
  Lines         542      542           
=======================================
  Hits          488      488           
  Misses         54       54

Impacted Files	Coverage Δ
src/ChainRulesCore.jl	`100.00% <ø> (ø)`
src/differentials/notimplemented.jl	`72.00% <ø> (ø)`
src/differentials/thunks.jl	`64.00% <ø> (ø)`
src/rules.jl	`100.00% <ø> (ø)`
src/differentials/abstract_differential.jl	`66.66% <50.00%> (ø)`
src/differentials/composite.jl	`83.05% <95.45%> (ø)`
src/differential_arithmetic.jl	`96.72% <100.00%> (ø)`
src/differentials/abstract_zero.jl	`94.11% <100.00%> (ø)`
src/differentials/one.jl	`83.33% <100.00%> (ø)`
src/rule_definition_tools.jl	`96.89% <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 b98e624...cf4d39a. Read the comment docs.

mzgubic · 2021-05-17T17:04:00Z

One had some issues on Julia 1.5 and 1.0, let's deprecate that in a separate PR.

nickrobinson251

So the full list of renaming is:

AbstractDifferential -> AbstractTangent
Composite{T} -> Tangent{T}
Zero -> ZeroTangent
DoesNotExist -> NoTangent

Thunk and InplaceableThunk are unchanged.

And the old names are deprecated, not yet removed, so this is non-breaking.

Should we have tests that these things are successfully deprecated (e.g. just @test Zero() === ZeroTangent())?

I'm okay with this renaming :)

EDIT: What's happening with One?

src/deprecated.jl

mzgubic · 2021-05-18T14:42:34Z

So the full list of renaming is:

AbstractDifferential -> AbstractTangent

Composite{T} -> Tangent{T}

Zero -> ZeroTangent

DoesNotExist -> NoTangent

Thunk and InplaceableThunk are unchanged.

And the old names are deprecated, not yet removed, so this is non-breaking.

Should we have tests that these things are successfully deprecated (e.g. just @test Zero() === ZeroTangent())?

I'm okay with this renaming :)

EDIT: What's happening with One?

Thanks for reviewing! I will add the deprecation tests.

I've left One for a separate PR since it was failing on julia 1.5 and 1.0.

Co-authored-by: Nick Robinson <npr251@gmail.com>

nickrobinson251 · 2021-05-18T15:05:43Z

I've left One for a separate PR since it was failing on julia 1.5 and 1.0.

Are those the version we test on? i.e. is it passing on some versions but not others?

I ask becasue I suspect we can't use @deprecate_binding for this? And we may need to use Base.depwarn in the One() constructor e.g. struct One; One() = (Base.depwarn("`One()` is deprecated; use `true` instead", :One); new()); end

mzgubic · 2021-05-18T15:30:25Z

I've left One for a separate PR since it was failing on julia 1.5 and 1.0.

Are those the version we test on? i.e. is it passing on some versions but not others?

yes

I ask becasue I suspect we can't use @deprecate_binding for this? And we may need to use Base.depwarn in the One() constructor e.g. struct One; One() = (Base.depwarn("`One()` is deprecated; use `true` instead", :One); new()); end

Correct, we can't use @deprecate_binding. I am happy to do the One deprecation PR as well, just wanted to separate it from this to not delay

rename definitions only

b5dc6ae

mzgubic commented May 13, 2021

View reviewed changes

nickrobinson251 reviewed May 13, 2021

View reviewed changes

simeonschaub approved these changes May 14, 2021

View reviewed changes

Miha Zgubic added 7 commits May 17, 2021 14:32

rename differential types

9ee7bd0

deprecate old bindings

4e4b207

deprecate One()

4396a12

fix deprecations

623a806

bump version

82d8446

fix docs

7d4346f

do not deprecate One

db1ef7e

mzgubic changed the title ~~WIP: rename differentials~~ rename differentials May 17, 2021

mzgubic mentioned this pull request May 17, 2021

rename differentials JuliaDiff/FiniteDifferences.jl#162

Merged

Miha Zgubic added 2 commits May 18, 2021 09:47

change back to InplaceableThunk

3beea2f

change back to Thunk

b442432

This was referenced May 18, 2021

rename differential types JuliaDiff/ChainRulesTestUtils.jl#153

Merged

rename differentials JuliaDiff/ChainRules.jl#413

Merged

nickrobinson251 approved these changes May 18, 2021

View reviewed changes

src/deprecated.jl Outdated Show resolved Hide resolved

mzgubic and others added 2 commits May 18, 2021 15:43

Update src/deprecated.jl

15e206c

Co-authored-by: Nick Robinson <npr251@gmail.com>

test deprecations

cf4d39a

mzgubic merged commit d31fd5b into master May 19, 2021

mzgubic deleted the mz/rename branch May 19, 2021 08:41

mzgubic mentioned this pull request May 19, 2021

rename ChainRules differential types FluxML/Zygote.jl#974

Merged

mzgubic mentioned this pull request May 19, 2021

rename ChainRules differential types JuliaDiff/Diffractor.jl#6

Closed

This was referenced May 27, 2021

Rename differential types dpsanders/ReversePropagation.jl#38

Merged

Re-export deprecated bindings simonster/Reexport.jl#28

Closed

mzgubic mentioned this pull request Jun 4, 2021

Rename ChainRulesCore differential types chrisbrahms/Hankel.jl#22

Merged

mzgubic mentioned this pull request Jun 23, 2021

Rename DoesNotExist #262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename differentials #353

rename differentials #353

mzgubic commented May 13, 2021 •

edited

Loading

mzgubic May 13, 2021

oxinabox May 13, 2021

mzgubic May 17, 2021

nickrobinson251 May 17, 2021

mzgubic May 18, 2021

mzgubic May 13, 2021

oxinabox May 13, 2021

oxinabox commented May 13, 2021 •

edited

Loading

nickrobinson251 May 13, 2021

oxinabox May 13, 2021

mzgubic commented May 14, 2021

simeonschaub left a comment

codecov-commenter commented May 17, 2021 •

edited

Loading

mzgubic commented May 17, 2021

nickrobinson251 left a comment •

edited

Loading

mzgubic commented May 18, 2021

nickrobinson251 commented May 18, 2021 •

edited

Loading

mzgubic commented May 18, 2021

	struct NoPossibleTangent <: AbstractZero end
	struct NoTangent <: AbstractZero end

rename differentials #353

rename differentials #353

Conversation

mzgubic commented May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox commented May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgubic commented May 14, 2021

simeonschaub left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 17, 2021 • edited Loading

Codecov Report

mzgubic commented May 17, 2021

nickrobinson251 left a comment • edited Loading

Choose a reason for hiding this comment

mzgubic commented May 18, 2021

nickrobinson251 commented May 18, 2021 • edited Loading

mzgubic commented May 18, 2021

mzgubic commented May 13, 2021 •

edited

Loading

oxinabox commented May 13, 2021 •

edited

Loading

codecov-commenter commented May 17, 2021 •

edited

Loading

nickrobinson251 left a comment •

edited

Loading

nickrobinson251 commented May 18, 2021 •

edited

Loading