Skip to content

Conversation

@oxinabox
Copy link
Member

@oxinabox oxinabox commented Apr 29, 2021

Closes #149
This is my preferred alternative to JuliaDiff/ChainRulesTestUtils.jl#142 (comment)

Note that it is not the case that if all my fields do not exist then i do not exist,
but it is the case that if all my fields are not perturbable then I am not pertablable.
Which is what DoesNotExist actually means, and we should rename it.

Composite{T}(; NamedTuple{field_names}(tangents)...)
end
else
return NO_FIELDS
Copy link
Member Author

Choose a reason for hiding this comment

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

The outer if here that gives us the NO_FIELDS would be able to be removed if we replaced NO_FIELDS with DoesNotExist().
Since then we would be in the case that all the fields (of which there are none) subtype DoesNotExist.
Vacuous truth.

JuliaDiff/ChainRulesCore.jl#175 (comment)

@mzgubic
Copy link
Member

mzgubic commented Apr 30, 2021

Also closes #153

@mzgubic
Copy link
Member

mzgubic commented Apr 30, 2021

Actually, it won't close any of the issues, because we still have two copies of rand_tangent (one here and one in ChainRulesTestUtils).

@oxinabox
Copy link
Member Author

Also closes #153

It won't close that

julia> rand_tangent(Random.GLOBAL_RNG, Colon())
ChainRulesCore.Zero()

It would close that if we also did
JuliaDiff/ChainRulesCore.jl#175 (comment)
which i think we should (which would also simplify the code in this)

Actually, it won't close any of the issues, because we still have two copies of rand_tangent (one here and one in ChainRulesTestUtils).

suffering

@mzgubic
Copy link
Member

mzgubic commented Apr 30, 2021

Oh yeah that's right, my bad. I am happy to move the code over and make it work. We discussed on slack a while back.

@oxinabox
Copy link
Member Author

oxinabox commented May 2, 2021

Thanks.
Please do so.
Either location is better than both

@mzgubic
Copy link
Member

mzgubic commented May 7, 2021

I think we can merge this now since JuliaDiff/ChainRulesTestUtils.jl#147 is in

@mzgubic
Copy link
Member

mzgubic commented May 7, 2021

Well, actually, needs a version bump, not sure why it does not show up as conflict.

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.

Should rand_tangent(2:3) return DoesNotExist()?

2 participants