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.

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

[SR-14218] [AutoDiff] Gradients not propagating out of member function #55999

Closed
porterchild opened this issue Sep 18, 2020 · 20 comments
Closed

[SR-14218] [AutoDiff] Gradients not propagating out of member function #55999

porterchild opened this issue Sep 18, 2020 · 20 comments
Assignees
@BradLarson
Labels
AutoDiff bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@porterchild
Copy link

Previous ID SR-14218
Radar None
Original Reporter @porterchild
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, AutoDiff
Assignee @BradLarson
Priority Medium

md5: de69ff0ec8ab8408330b2613882418fa

Issue Description:

In the below code, the gradient values propagate unexpectedly. Specifically, the self.number tangent vector disappears after leaving the scope of inner() (on the reverse pass). Removing the useless loop results in correct gradients.

import _Differentiation

struct MZ: Differentiable{
    var number: Float = 3
    var oneList: [Float] = [1]
    var anotherList: [Float] = [1]
    
    mutating func outer(){
        self = self.withDerivative {
            print("gradients actually coming out of inner ", $0)
        }
        inner()
    }
    
    mutating func inner(){
        self = self.withDerivative{ print("gradients that should come out of inner", $0, "\n")}
                
        self.oneList = [self.number]
        self.anotherList = [self.anotherList[0] + self.oneList[0]]
        
        for _ in 0..<1 {} //if you get rid of this useless loop, the gradients are correct
    }
}

func loss(htm: MZ) -> Float{
    var htm = htm
    htm.outer()
    return htm.anotherList[0]
}

var htm = MZ()
let gradFunc = gradient(of: loss)
let grad = gradFunc(htm)

//gradients that should come out of inner TangentVector(number: 1.0, oneList: [], anotherList: [1.0]) 
//gradients actually coming out of inner  TangentVector(number: 0.0, oneList: [], anotherList: [1.0])
@porterchild
Copy link
Author

Ping @rxwei

@rxwei
Copy link
Member

rxwei commented Feb 13, 2021

I'll take a look some time later. I'll need to convert all these AD bugs starting with "TF-" to SR ones. Some help with that would be appreciated too!

@porterchild
Copy link
Author

Sure! There might be a minimally manual way to do it via the "Export" option at the top right of the page... I don't see an import option however.

@porterchild
Copy link
Author

Any preferences about converting them? With or without comments? Is order important? Close all the converted TF bugs I assume? Preserve blocking/related/subtask relationships? Etc.

@rxwei
Copy link
Member

rxwei commented Feb 15, 2021

You can click "More" -> "Move" to convert these issues to SR. It will keep all the existing info and just change the bug number.

@porterchild
Copy link
Author

🙂 that sounds more sensical.

@porterchild
Copy link
Author

How many/which of the TF issues do you want to convert?

@rxwei
Copy link
Member

rxwei commented Feb 15, 2021

All of AD-related bugs.

@rxwei
Copy link
Member

rxwei commented Feb 15, 2021

If it's not too much trouble, please add an "AutoDiff" label if there isn't one. Thanks so much, @porterchild!

@porterchild
Copy link
Author

Glad to help!

@porterchild
Copy link
Author

Including Closed issues, @rxwei?

@rxwei
Copy link
Member

rxwei commented Feb 15, 2021

Just the open ones.

@porterchild
Copy link
Author

Include ones like https://bugs.swift.org/browse/TF-4? Not a bug, but directly to do with autodiff...

Does order matter? I was going to start back at the beginning.

@rxwei
Copy link
Member

rxwei commented Feb 16, 2021

I'll go ahead and close TF-4 since it's complete.

@rxwei
Copy link
Member

rxwei commented Feb 16, 2021

Let's sync issues that are related specifically to the AD feature, not the S4TF-specific ones such as planning/process. Thanks again!

@porterchild
Copy link
Author

Looks like there some of the earlier issues may be stale, so I'll start around TF-700. I'm just going to convert the ones that are obviously bugs/crashes (and leave out everything else). Let me know if you want to be more/less inclusive than that.

@rxwei
Copy link
Member

rxwei commented Feb 16, 2021

Sounds good. Thank you!

@porterchild
Copy link
Author

👍

@porterchild
Copy link
Author

OK I should have gotten most of them from TF-700 on. I can go back further than that if you like. Let me know if you want to convert any other types of tickets

@BradLarson
Copy link
Collaborator

Should be resolved by PR #37861.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BradLarson BradLarson

Labels
AutoDiff bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rxwei @BradLarson @porterchild