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

fixed observation noise bug #2

Merged
merged 1 commit into from
Mar 7, 2021
Merged

Conversation

KrisNguyen135
Copy link
Contributor

When declaring the posterior approximation method in preference.py, s_eval now takes the default value of alpha.

When computing the matrix c in posterior.py, the second-order derivative term is subtracted from the off-diagonal entries and added to the diagonal entries, following Chu and Ghahramani, 2005.

@codecov-io
Copy link

Codecov Report

Merging #2 (3a169da) into master (575fdd8) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #2      +/-   ##
==========================================
- Coverage   96.10%   96.09%   -0.02%     
==========================================
  Files           6        6              
  Lines         385      384       -1     
  Branches       47       47              
==========================================
- Hits          370      369       -1     
  Misses          5        5              
  Partials       10       10              
Impacted Files Coverage Δ
GPro/posterior.py 95.94% <100.00%> (-0.06%) ⬇️
GPro/preference.py 94.94% <100.00%> (ø)

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 575fdd8...3a169da. Read the comment docs.

@chariff
Copy link
Owner

chariff commented Mar 7, 2021

Thank you for catching and correcting my mistakes. I went through Chu and Ghahramani, 2005 and it seems that I also forgot to divide the first order derivative of the loss function by the noise variance in https://github.com/chariff/GPro/blob/master/GPro/posterior.py#L134.

@chariff chariff merged commit 9b02362 into chariff:master Mar 7, 2021
@KrisNguyen135
Copy link
Contributor Author

Thanks for merging the pull request.

Ha! Indeed, there should be a sigma there which I also missed. Will make a new pull request shortly.

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.

3 participants