Skip to content

Replace NO_FIELDS by NoTangent() #163

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 1, 2021
Merged

Replace NO_FIELDS by NoTangent() #163

merged 7 commits into from
Jun 1, 2021

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented May 27, 2021

@oxinabox
Copy link
Member

oxinabox commented May 27, 2021 •
edited
Loading

this will close #153

@oxinabox oxinabox linked an issue May 27, 2021 that may be closed by this pull request
rand_tangent(::Colon) should return DoesNotExist() #153
Closed
@mzgubic
Copy link
Member Author

mzgubic commented May 28, 2021

Actually I think this is breaking because it changes the rand_tangent for a struct with no fields

@oxinabox
Copy link
Member

Actually I think this is breaking because it changes the rand_tangent for a struct with no fields

I think that is a bug-fix. It is the thing that makes Colon() have the correct behavour

Hmm on that basis the change to NO_FIELDS might actually be a bug fix, and not a breaking change

oxinabox
oxinabox reviewed May 28, 2021
View reviewed changes
@mzgubic
Copy link
Member Author

mzgubic commented Jun 1, 2021

Actually I think this is breaking because it changes the rand_tangent for a struct with no fields

I think that is a bug-fix. It is the thing that makes Colon() have the correct behavour

Hmm on that basis the change to NO_FIELDS might actually be a bug fix, and not a breaking change

I am coming around to think that this PR can be passed as a bug fix, but not the ChainRulesCore one (since it affects too many downstream packages that were relying on this "buggy" behaviour)

@mzgubic mzgubic merged commit d3ac3c4 into master Jun 1, 2021
@mzgubic mzgubic deleted the mz/nofields branch June 1, 2021 11:32
@mzgubic mzgubic mentioned this pull request Jun 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oxinabox oxinabox oxinabox 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.

rand_tangent(::Colon) should return DoesNotExist()

2 participants

@mzgubic @oxinabox