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

Error inside Tangent constructor if incorrect backing type is used #495

Merged
merged 8 commits into from
Oct 19, 2021

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Oct 19, 2021

Closes #494. Might need to fix things downstream if incorrect types are used.

src/tangent_types/tangent.jl Outdated Show resolved Hide resolved
test/tangent_types/tangent.jl Outdated Show resolved Hide resolved
test/tangent_types/tangent.jl Outdated Show resolved Hide resolved
test/tangent_types/tangent.jl Outdated Show resolved Hide resolved
test/tangent_types/tangent.jl Outdated Show resolved Hide resolved
test/tangent_types/tangent.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@oxinabox
Copy link
Member

oxinabox commented Oct 19, 2021

address my comments as you will, and make sure CI is passing.
then merge when happy

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mzgubic
Copy link
Member Author

mzgubic commented Oct 19, 2021

Hmm, actually - what should we do about Any primal type? My initial thought was that it should be a Tuple (since there are no names), but we could allow Tuple, NamedTuple, or Dict?

@oxinabox
Copy link
Member

Hmm, actually - what should we do about Any primal type? My initial thought was that it should be a Tuple (since there are no names), but we could allow Tuple, NamedTuple, or Dict?

Yes.
Or even allowing utterly anything.
It's basically just used as a hack in Zygote, but let's not break that hack.

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2021

Codecov Report

Merging #495 (1ce68f8) into main (bde3166) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #495      +/-   ##
==========================================
- Coverage   92.94%   92.90%   -0.04%     
==========================================
  Files          15       15              
  Lines         822      818       -4     
==========================================
- Hits          764      760       -4     
  Misses         58       58              
Impacted Files Coverage Δ
src/tangent_types/tangent.jl 85.18% <100.00%> (+0.81%) ⬆️
src/tangent_types/thunks.jl 94.89% <0.00%> (-0.11%) ⬇️
src/accumulation.jl 97.14% <0.00%> (-0.08%) ⬇️
src/projection.jl 97.40% <0.00%> (-0.08%) ⬇️
src/rule_definition_tools.jl 96.25% <0.00%> (-0.03%) ⬇️

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 bde3166...1ce68f8. Read the comment docs.

@mzgubic
Copy link
Member Author

mzgubic commented Oct 19, 2021

Diffractor (same failure on its master) and Julia nightly (test unexpectedly passes) errors look unrelated. Will merge if ChainRules tests pass

Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
@simeonschaub
Copy link
Member

This broke Diffractor. Is this really worth it?

@mcabbott
Copy link
Member

It could be written out using dispatch instead of if statements, maybe that would help?

@simeonschaub
Copy link
Member

No, that's not the problem. Diffractor uses tuple backing for tangents of tangents over tuples. This could probably be changed at some point, but I don't think this should have been merged without any discussion now that we have the integration test.

@oxinabox
Copy link
Member

No, that's not the problem. Diffractor uses tuple backing for tangents of tangents over tuples.

Hmm I am not sure that it should.

But for now maybe let's revert this PR, and then we can think about it.

I don't think this should have been merged without any discussion now that we have the integration test.

Ah sorry, I think we just got used to Diffractor being broken.
But it works now that it's CI is set to use Julia nightly.

Yes, this was bad.

@simeonschaub
Copy link
Member

simeonschaub commented Oct 28, 2021

Just for the future: Could you please always squash merge commits like these before merging? It's just such a pain to revert otherwise. (Edit: Turns out it's not that bad if you are working off the correct branch 🤦. Squash merges are still pretty much always easier to work with though, so my point still stands.) I wonder whether we should just disallow anything except squash merge for this repo.

simeonschaub added a commit that referenced this pull request Oct 28, 2021
This reverts commit a724bbb, reversing
changes made to bde3166.
@simeonschaub simeonschaub mentioned this pull request Oct 28, 2021
@mzgubic
Copy link
Member Author

mzgubic commented Oct 28, 2021

Hey Simeon, sorry about breaking Diffractor. The error looked unrelated and was the same one as on master at the time, so I didn't think there was anything wrong with the PR. And yes, will make sure to merge next time.

No, that's not the problem. Diffractor uses tuple backing for tangents of tangents over tuples.

Are there any reasons why a NamedTuple, e.g. Tangent{Tangent}(; backing=(1, 2, 3)) would not work? I think that would be the most consistent thing to do, unless there are good reasons to just use a Tuple?

@simeonschaub
Copy link
Member

Are there any reasons why a NamedTuple, e.g. Tangent{Tangent}(; backing=(1, 2, 3)) would not work?

I'm pretty sure that's possible, but it's not super trivial because that assumption is used in multiple places. Once I have figured that out, we should be able to relate this.

@mcabbott
Copy link
Member

This is JuliaDiff/Diffractor.jl#67, BTW.

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 we restrict the types that Tangent can be backed by?
5 participants