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

tpcf jackknife covariance matrix normalization error #815

Closed
duncandc opened this Issue Oct 11, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@duncandc
Copy link
Contributor

duncandc commented Oct 11, 2017

The covariance matrices in the tpcf_jackknife() mock_observables function aren't correctly normalized.

The tpcf_jackknife() function uses the numpy.cov() function to calculate the covariance matrix which normalizes by default using 1/(N-1) where N is the number of samples. For jackknife samples, it should instead be normalized by (N-1)/N.

Thanks to @yymao for the consultation on this matter.

@duncandc duncandc self-assigned this Oct 11, 2017

@aphearin aphearin added bug and removed bug-fix labels Oct 11, 2017

@aphearin

This comment has been minimized.

Copy link
Contributor

aphearin commented Oct 18, 2017

CC @tmcclintock - you should be aware of this bug. We are fixing asap. I will re-release v0.6 of the code very soon, possibly even this week, next week for sure. Until then, after the bugfix PR, you can use the bleeding edge version by cloning master and building from source. Apologies if this bug caused you problems in your work.

@tmcclintock

This comment has been minimized.

Copy link
Contributor

tmcclintock commented Oct 18, 2017

I'm pretty sure this is the line, as well as the following two, that need fixing.

According to the numpy docs you need to use either the bias argument or the ddof argument. If you use bias then (without having checked this myself...) you will reproduce what you have now if you use bias = Nsub**3 - 1, and the correct behavior is obtained if you use bias = (Nsub**3 - 1.)/Nsub**3.

Apologies for being too lazy to PR.

@aphearin

This comment has been minimized.

Copy link
Contributor

aphearin commented Oct 18, 2017

No apologies necessary, Tom, this is super helpful. Many thanks!

@tmcclintock

This comment has been minimized.

Copy link
Contributor

tmcclintock commented Oct 18, 2017

Oops it isn't Nsub**3, it is the product of all the the elements of Nsub.

@duncandc

This comment has been minimized.

Copy link
Contributor Author

duncandc commented Oct 18, 2017

Right, I believe bias must be a boolean. Take a look at the current fix. I believe we are in agreement.

@tmcclintock

This comment has been minimized.

Copy link
Contributor

tmcclintock commented Oct 18, 2017

Ah true. I misread the numpy docs.

@aphearin

This comment has been minimized.

Copy link
Contributor

aphearin commented Oct 19, 2017

Resolved by PR #818

@aphearin aphearin closed this Oct 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment