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

Redefine the N_K parameter per log10 bin #240

Merged
merged 3 commits into from
Oct 6, 2017
Merged

Conversation

wagoner47
Copy link
Contributor

I switched the definition of nk in all the places I could find it (I found 3 instances) in src/ccl_power.c to be the number of decades times N_K now. I used ceil to safely cast it to an integer in the case of non-integer number of decades. I also updated the default value of N_K in the parameter file so that it would give as close to 1000 bins as possible for the default kmin and kmax.

…as close as possible to 1000 for default kmin and kmax
@elisachisari
Copy link
Collaborator

All checks pass and in principle looks good. I identified a 1e-3 difference of the output power spectra at high-h. Here is an example. We need to check more systematically whether varying N_K per bin gives different results at high k.
screen shot 2017-07-14 at 7 40 36 pm

Copy link
Contributor

@tmcclintock tmcclintock left a comment

Choose a reason for hiding this comment

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

I'm going to approve. Nice job @wagoner47 this is very helpful.

@tmcclintock
Copy link
Contributor

@elisachisari you can review this very quickly and merge it if you want.

@elisachisari
Copy link
Collaborator

@tmcclintock I looked at this some time ago. The problem is that we are not passing the 1e-4 requirement with this implementation. This is part of a bigger problem of convergence of the splines and extrapolation scheme at high-k, however. I'm happy for this to be merged, bearing in mind that the convergence issue needs to be revisited in a wider context (issue #258). I merged master into this branch and all checks pass. The plots in the note discussing spline convergence will become outdated after this PR is merged.

Copy link
Collaborator

@elisachisari elisachisari left a comment

Choose a reason for hiding this comment

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

I approve of this new implementation of N_K. I merged master on this branch and re-defined N_K in the note. This PR means that the spline convergence plots are now outdated in the CCL note and should be revisited in the context of issue #258.

@elisachisari elisachisari merged commit 9fdce07 into master Oct 6, 2017
@elisachisari elisachisari deleted the redefine_n_k branch January 23, 2018 16:21
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.

None yet

3 participants