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

A couple of possible issues with cA code #164

Open
gsanroma opened this issue Aug 11, 2016 · 3 comments
Open

A couple of possible issues with cA code #164

gsanroma opened this issue Aug 11, 2016 · 3 comments

Comments

@gsanroma
Copy link

Hi, there might be a couple of issues in the cA code.
First, when computing the loss due to the jacobian (line 211) shouldn't the integer division (//) be replaced by a a true division (/) ?
Second, the result of the previous line of code is a scalar. Then, why taking its mean when computing the cost function in next line of code (218) ?
Thanks,
Gerard.

@lamblin
Copy link
Member

lamblin commented Aug 11, 2016

First, when computing the loss due to the jacobian (line 211) shouldn't the integer division (//) be replaced by a a true division (/) ?

Yes, this should be a true division. This seems to be an issue that has been introduced when trying to make the code compatible with python 3, but that line should not have been changed, since J (and then sum(J ** 2)) should be float.

A pull request to fix that would be welcome.

Second, the result of the previous line of code is a scalar. Then, why taking its mean when computing the cost function in next line of code (218) ?

It is indeed unnecessary, but should not affect the computation speed. That fix would be welcome as well.

Thanks for the report.

@gsanroma
Copy link
Author

Is there any tutorial on how to create pull requests ?

@lamblin
Copy link
Member

lamblin commented Aug 12, 2016

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

No branches or pull requests

2 participants