Skip to content

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Dec 17, 2020

Closes JuliaDiff/ChainRulesCore.jl#266

There were two issues:

  1. missing ;kwargs causing the wrong method to dispatch
  2. finite differencing returns a Tuple for functions which output a Tuple

@oxinabox
Copy link
Member

Needs tests.
We can make a MWE of a rule for x->(x, x) where test_frule currently errors/fails

@mzgubic
Copy link
Member Author

mzgubic commented Dec 17, 2020

right, good to keep the test inside ChainRulesTestUtils and not rely on integration to flag this.

I needed to do x->(x, 1.0) because it is important for the backing type of the composite to be different from the differential so that it doesn't _can_pass_early

src/testers.jl Outdated
return to_composite(jvp(fdm, f2, sigargs...))
end

# remove after https://github.com/JuliaDiff/FiniteDifferences.jl/issues/97
Copy link
Member

Choose a reason for hiding this comment

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

using #TODO causes it to get picked up by various automated tooling

Suggested change
# remove after https://github.com/JuliaDiff/FiniteDifferences.jl/issues/97
#TODO remove after https://github.com/JuliaDiff/FiniteDifferences.jl/issues/97

Copy link
Member

Choose a reason for hiding this comment

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

also maybe a comment about this fixing up FiniteDiff sometimes returning Tuples etc rathter than valid diff types as part of that TODO.
to explai what this function does?

src/testers.jl Outdated
Comment on lines 104 to 106
to_composite(x::Tuple) = Composite{typeof(x)}(x...)
to_composite(x::NamedTuple) = Composite{typeof(x)}(;x...)
to_composite(x) = x
Copy link
Member

Choose a reason for hiding this comment

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

Since they are not expored

Suggested change
to_composite(x::Tuple) = Composite{typeof(x)}(x...)
to_composite(x::NamedTuple) = Composite{typeof(x)}(;x...)
to_composite(x) = x
_to_composite(x::Tuple) = Composite{typeof(x)}(x...)
_to_composite(x::NamedTuple) = Composite{typeof(x)}(;x...)
_to_composite(x) = x

Might want to say:

Suggested change
to_composite(x::Tuple) = Composite{typeof(x)}(x...)
to_composite(x::NamedTuple) = Composite{typeof(x)}(;x...)
to_composite(x) = x
_maybe_as_composite(x::Tuple) = Composite{typeof(x)}(x...)
_maybe_as_composite(x::NamedTuple) = Composite{typeof(x)}(;x...)
_maybe_as_composite(x) = x

or something?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe including fix in the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

all good suggestions, so added them all

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.

Integration tests fail
2 participants