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

Project nothing to nothing #1075

Closed
wants to merge 3 commits into from
Closed

Project nothing to nothing #1075

wants to merge 3 commits into from

Conversation

DhairyaLGandhi
Copy link
Member

@DhairyaLGandhi
Copy link
Member Author

DhairyaLGandhi commented Sep 23, 2021

This is likely to break some tests, but seems like the cogent thing to do for some cases.

@oxinabox
Copy link
Member

I am not sure if this is the right place for this fix.
Or if we need a more general insertion of the code to convert CRC types in to Zygote types.
I think I saw it happening with other types than just Nothing.

@mcabbott should review.

@DhairyaLGandhi
Copy link
Member Author

DhairyaLGandhi commented Sep 24, 2021

Since _project is the last transform Zygote makes now, it should generically output Zygote types. That way, we need a conversion layer between _project and ProjectTo to go from CR types to Zygote's.

@@ -106,6 +106,7 @@ Convert `x` from the differentials types ChainRules uses to the format Zygote us
# Zygote convention: even if many AbstractZero partials (i.e. multi-input function), make just 1 nothing.
@inline wrap_chainrules_output(x::Tuple{Vararg{ChainRules.AbstractZero}}) = nothing
@inline wrap_chainrules_output(x::ChainRules.AbstractZero) = nothing
@inlint wrap_chainrules_output(x::ChainRules.NoTangent) = nothing
Copy link
Member

@mcabbott mcabbott Sep 24, 2021

Choose a reason for hiding this comment

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

This seems completely safe to me. Although might be covered by the AbstractZero line above?

Re PR's original change c53df99 to propagate nothings, I was surprised this broke no tests. I don't recall now what test motivated that method in the first place, and it's possible that some later change removed the need.

Does this solve SciML/SciMLSensitivity.jl#493? As noted there, I can't reproduce this locally.

@DhairyaLGandhi
Copy link
Member Author

DhairyaLGandhi commented Sep 24, 2021

ref SciML/SciMLSensitivity.jl#493 (comment)

I believe we have the _project in the getindex adjoint setup to handle OneElement which doesn't have information about the type of the array. Why do we have OneElement? Our accumulation speedup came from generalising accum, and incrementally through OneElement. Judging by the few number of times it coming up in issues, it might be that we can have a more stable and simpler implementation by rethinking it somewhat.

@CarloLucibello
Copy link
Member

closing as stale, possibly also not needed anymore

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.

None yet

4 participants