Fix for issue #1191 #1202
Fix for issue #1191 #1202
Conversation
…arameters are non-zero but transfer function isn't isitgr
There was a problem hiding this comment.
This is generally a good fix, thank you @sankarshana16. There is one thing regarding the values of c1_mg and c2_mg which needs to be sorted out, this is what is causing the unit tests to fail.
Also just have a look at the flake8 results - you will need to format the new lines of code to pass those formatting requirements before merging.
I wasn't able to test on my local machine yet because of some weird installation issues (which also happen on the master branch so I think it's me, not this branch). I'll try to fix that in the meantime while these fixes are implemented.
| @@ -294,6 +294,16 @@ def __init__( | |||
| modified_gravity.MuSigmaMG): | |||
| raise NotImplementedError("`mg_parametrization` only supports the " | |||
| "mu-Sigma parametrization at this point") | |||
| """ | |||
There was a problem hiding this comment.
I am assuming this commented out section is leftover from during modifications before realising you only want to through the error when c1_mg, c2_mg or lambda_mg is non-zero. Remove commented out section before merging.
pyccl/cosmology.py
Outdated
| if(c1_mg != 0. or c2_mg !=0. or lambda_mg != 0.): | ||
|
|
||
| if isinstance(self.mg_parametrization, modified_gravity.MuSigmaMG) and self.transfer_function_type is not 'boltzmann_isitgr': | ||
| raise ValueError("mu-Sigma parametrization is inconsistent with your transfer function choice (required to use isitgr)") |
There was a problem hiding this comment.
I think it would be better to have a slightly more informative error message. This one doesn't clarify to the user that the issue is the scale-dependence parameters specifically. Can it say something like "Your choice of c1_mg, c2_mg, and lambda_mg values is inconsistent with your transfer function choice (you must choose istigr)."?
pyccl/cosmology.py
Outdated
| @@ -471,6 +481,11 @@ def _build_parameters( | |||
| c2_mg = self.mg_parametrization.c2_mg | |||
| lambda_mg = self.mg_parametrization.lambda_mg | |||
|
|
|||
| if(c1_mg != 0. or c2_mg !=0. or lambda_mg != 0.): | |||
There was a problem hiding this comment.
The default values of c1_mg and c2_mg are actually 1. GR is recovered when c1_mg = c2_mg=1 (regardless of lambda value) or when c1_mg=c2_mg=lambda_mg=0. So, need a slightly more complicated check here to cover both cases. This is why this branch is currently failing a bunch of unit tests I think.
There was a problem hiding this comment.
@c-d-leonard I've modified the code to only allow c1_mg = c2_mg = 1 or c1_mg = c2_mg = lambda_mg = 0., but complain otherwise with a better error message as you suggested. To be honest, I still had a few unit test problems, but from what I could see, they were not related to this (I think). Let me know if this is fine
|
@sankarshana16 are you able to address these? |
|
@c-d-leonard |
Pull Request Test Coverage Report for Build 11973380207Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Modified cosmology.py so that it complains when mu-sigma scale-dependent parameters are non-zero but transfer function isn't the isitgr one