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

Regularization & standardization #111

Merged
merged 15 commits into from
Apr 16, 2021
Merged

Regularization & standardization #111

merged 15 commits into from
Apr 16, 2021

Conversation

mhowlan3
Copy link
Contributor

  1. Log-posterior calculation in MCMC
  2. SVD regularization via truncation
  3. Re-normalization (standardization) of output space

@mhowlan3 mhowlan3 requested a review from odunbar March 23, 2021 15:47
full_cov = Diagonal(gvar)
eigs = eigvals(full_cov)
log_gpfidelity = -FT(0.5) * sum(log.(eigs))
# Combine got log_rho
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this version of MCMC, we are actually assuming that gvar is a vector of floats, I'm thinking maybe we should relax this to take in matrices. Perhaps the easiest way is to rewrite the current mcmc_sample! function to take in matrix input gcov and add this extra function

function mcmc_sample!(mcmc::MCMC{FT}, g::Vector{FT}, gvar::Vector{FT}) where {FT}
    return mcmc_sample!(mcmc, g, Diagonal(gvar))
end

Copy link
Collaborator

@odunbar odunbar Mar 24, 2021

Choose a reason for hiding this comment

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

I've just checked that if eigvals receives a Diagonal type matrix it really does just take the diagonal as the eigenvalues. and so is a nanosecond timed computation still, and so the above should still take advantage of the structure

Copy link
Contributor Author

@mhowlan3 mhowlan3 Mar 25, 2021

Choose a reason for hiding this comment

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

Great! I also wrote it this way in the event that full_cov becomes a full matrix in the future, rather than just diagonal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure you have pushed this yet? I can't see it it

@CliMA CliMA deleted a comment from codecov bot Apr 15, 2021
@@ -66,11 +76,13 @@ struct GaussianProcess{FT<:AbstractFloat, GPM}
"the Gaussian Process (GP) Regression model(s) that are fitted to the given input-data pairs"
models::Vector
"the singular value decomposition of obs_noise_cov, such that obs_noise_cov = decomposition.U * Diagonal(decomposition.S) * decomposition.Vt."
decomposition::Union{SVD, Nothing}
decomposition::Union{SVD, decomp_struct, Nothing}
Copy link
Collaborator

@odunbar odunbar Apr 15, 2021

Choose a reason for hiding this comment

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

It's fine to create the decomposition structure here,
might be nice to save everything in you decomp_struct format, then you can use Union{decomp_struct, Nothing} as the type.

You just need to add a constructor for decomp_struct for the non-truncated case, (something like this?)

decomp_struct(svd::SVD) = decomp_struct(svd.V, svd.Vt, svd.S, size(svd.V)[0])

and then create the structs

decomposition = decomp_struct(svd(obs_noise_cov))

Copy link
Collaborator

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

Thanks very much Mike!

  1. If you agree about the decomposition type, could you change that, should be a quick change
  2. Check that you wish to keep all the println statements in the source files, and also there are a couple of # TO DO comments that could be removed in the source files (check the diff)
  3. Codecov report (if you go through the testing) shows a +1.43% increase

Otherwise LGTM!

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #111 (c73e8c4) into master (bde23f5) will decrease coverage by 8.08%.
The diff coverage is 38.18%.

❗ Current head c73e8c4 differs from pull request most recent head abc2dec. Consider uploading reports for the commit abc2dec to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
- Coverage   88.98%   80.90%   -8.09%     
==========================================
  Files           4        4              
  Lines         336      377      +41     
==========================================
+ Hits          299      305       +6     
- Misses         37       72      +35     
Impacted Files Coverage Δ
src/GaussianProcessEmulator.jl 75.68% <32.60%> (-12.30%) ⬇️
src/MarkovChainMonteCarlo.jl 84.29% <66.66%> (-2.66%) ⬇️

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 bde23f5...abc2dec. Read the comment docs.

@mhowlan3
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Apr 15, 2021
111: Regularization & standardization r=mhowlan3 a=mhowlan3

1. Log-posterior calculation in MCMC
2. SVD regularization via truncation
3. Re-normalization (standardization) of output space

Co-authored-by: mhowlan3 <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 15, 2021

Timed out.

@mhowlan3
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 16, 2021

@bors bors bot merged commit 8723d4a into master Apr 16, 2021
@bors bors bot deleted the mh/regularization branch April 16, 2021 02:58
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