LSSTDESC / CCL

DESC Core Cosmology Library: cosmology routines with validated numerical accuracy
BSD 3-Clause "New" or "Revised" License
145 stars 68 forks source link

Silent failure for P(k) rescaling in cosmologies scale-dependent MG #1191

Open c-d-leonard opened 4 months ago

c-d-leonard commented 4 months ago

CCL will allow you to define a cosmology with a scale-dependent mu / Sigma parameterization, and ask for a 'boltzmann_camb' or 'boltzmann_class' transfer function (rather than an 'isitgr') transfer function. If you do this, CCL will re-scale the power spectrum for you using a growth function computed from solving the quasistatic growth equation with mu. But, it will do something where it seems to basically ignore the scale-dependence, silently (see the k=0 case in ccl_mu_MG in ccl_muSigma.c - this is how it is called in this scenario from within growth_ode_system_muSig in ccl_background.c.

It doesn't really make physical sense to do this rescaling option when you have a scale-dependent mu anyway - you need to solve the full Boltzmann equations. We should throw an error if somebody defines a cosmology with these parameters and transfer function combination then tries to get the matter power spectrum.

sankarshana16 commented 4 months ago

Hi, I've been looking at the code today. One thing that struck me is that even for the time-dependent case, you'd still want to use the isitgr transfer function, if you want to compute the weak-lensing Cls correctly right? It's true that you can compute the linear P(k) correctly by simply rescaling by the growth function. But I don't see any similar kind of re-scaling by \Sigma (if mu_Sigma is called) in the angular_cl routine. This means you'd have to do it manually, which can be ugly and probably the sort of thing we want to avoid anyway. So, I think it's just better to raise an error if any transfer function other than the isitgr one is called. Would you agree? If yes, I've added an extra if statement that does this and I think it seems to be working on my machine

c-d-leonard commented 4 months ago

Thanks for looking at this. I don't think I agree though. Two reasons:

I'll tag @damonge as well for his thoughts?

sankarshana16 commented 4 months ago

Ah, okay. I was looking in the wrong place for this, my bad! What you're saying makes sense, I'll modify at the appropriate place to only complain if the parameters that control scale-dependence are non-zero

sankarshana16 commented 2 months ago

I've added a few lines of code that essentially checks that the transfer function that is used is the isitGR one when the parameters that control scale-dependence are non-zero. If they are non-zero but the transfer function isn't the isitGR one, it errors out. I tested it in a notebook. Would you like to check, before I push it?

damonge commented 1 week ago

@c-d-leonard I didn't quite understand what solution you are proposing here. Should we throw an error when isitgr is not being used or do you want to fix the current implementation to take into account the scale dependence of mu and sigma properly when using camb/class (I'd need to understand what this implies -- which is already a sign that it should be probably someone else who addresses this issue)

c-d-leonard commented 2 days ago

@damonge I think the solution I was proposing is that which @sankarshana16 has implemented: throw an error when the scale-dependence parameters are non-zero / non-GR values and the isitgr transfer function is not being used. But if the scale-dependence parameters are zero / GR, let the user go ahead with the camb / class method. I will review the Pull Request from Shankar now.