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

added normal Glorot initialization #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vcadillog
Copy link

Hi, great work with this golang binding; I was translating my code for production but I saw the xavier_normal_ method was missing.
Hope this can help other people who had encountered this issue.

//
// Paper: https://proceedings.mlr.press/v9/glorot10a/glorot10a.pdf
// Pytorch implementation: https://github.com/pytorch/pytorch/blob/df50f91571891ec3f87977a2bdd4a2b609d70afc/torch/nn/init.py#L337
func XavierNormal_(x *ts.Tensor, gainOpt ...float64) {
Copy link
Owner

Choose a reason for hiding this comment

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

  • We try to use Go conventional naming. If it's a unexposed function we can start with lowercase xavierNormal()
  • There a placeholder for GolorotInit at
    // glorotInit :
    . It would be great if you could fill in those things.
  • Try some unit test or example (make sure accuracy and no memory leak are the most priority as the function will be used everywhere (I often test result against Python in terms of accuracy).

@sugarme
Copy link
Owner

sugarme commented Oct 15, 2023

@vcadillog ,

Thanks for the PR. Please see my comments/wishes above. Would love to have this contribution. Please take your time.

@vcadillog
Copy link
Author

Yes, I noticed that Glorot initialization needed to be implemented; and also I'm not quite sure if I should use lowercase because the xavier_uniform_ is defined as XavierUniform_ in the binding; but if you are referring to the glorotNInit, yes; I'll make sure to use lowercase.

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.

2 participants