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

More explicit gradient state interface #113

Merged
merged 5 commits into from Nov 3, 2022

Conversation

asilvas
Copy link
Contributor

@asilvas asilvas commented Nov 3, 2022

Clear naming is subjective, but tried to be more explicit.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 3, 2022
@yushiyangk
Copy link
Contributor

yushiyangk commented Nov 3, 2022

Can we also rename the variable from grad to something else perhaps context. In other places grad refers to a Tensor instead.

Personally, in the context of a backwards pass, I feel that grad_result is more confusing than grad_in.

@bwasti
Copy link
Contributor

bwasti commented Nov 3, 2022

Thanks for bringing this up!

I agree that context or ctx would be better than grad.

Here's my proposal for names:

export interface GradContext {
   forward_inputs: [Tensor, ...ArgType[]]
   forward_output: Tensor
   backward_input: Tensor // the associated gradient of forward_output
   backward_output_index: number // index of the associated forward input to be differentiated
 }

Copy link
Contributor

@bwasti bwasti left a comment

Choose a reason for hiding this comment

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

LGTM! one nit with the change to leakyRelu

@@ -16,7 +16,7 @@ export function relu(tensor: Tensor): Tensor {
return tensor.maximum(scalar(0))
}

export function leakyRelu(tensor: Tensor, negative_slope: number): Tensor {
export function leakyRelu(tensor: Tensor, negative_slope = 1e-3): Tensor {
Copy link
Contributor

Choose a reason for hiding this comment

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

fine to sneak this in, but can you annotate with the type? negative_slope: number = 1e-3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bun format removes the type annotation when the arg has a default

Copy link
Contributor

Choose a reason for hiding this comment

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

:o @cryptodeal is that expected?

@bwasti
Copy link
Contributor

bwasti commented Nov 3, 2022

Here's an updated diagram with the new names:

out

@bwasti bwasti merged commit 9158fba into facebookresearch:main Nov 3, 2022
@asilvas asilvas deleted the grad-interface branch November 3, 2022 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants