Work on CCL module for cross-correlations #16

Pablo-Lemos · 2021-04-20T14:29:59Z

No description provided.

cmbant · 2021-04-20T14:54:31Z

If ccl is going to be loaded from the global init, should probably make pyccl load dynamically (otherwise it will error if people don't have pyccl installed - e.g. anyone on Windows).

There seem to be two Tester classes here, can the one in ccl.py be deleted?

Pablo-Lemos · 2021-04-20T15:36:31Z

Done (I think)

timothydmorton

See the various inline comments-- main point is to give the likelihood in ccl.py a name, make it inherit from GaussianLikelihood, and expand the testing. See some of the guidelines here for guidance; also feel free to ask me for clarification on any of these points. I've also just today turned on the github actions CI to run on pull requests (it wasn't on before), so we need to make sure those pass.

timothydmorton · 2021-04-20T17:31:55Z

soliket/__init__.py

@@ -3,4 +3,4 @@
 from .ps import PSLikelihood, BinnedPSLikelihood
 from .clusters import ClusterLikelihood
 from .mflike import MFLike
-from .ccl import CCL
+from .ccl import CCL, Tester


Why is Tester imported at the package top level like this? If it's something just for testing, it should just be in the test module, not imported here.

I also prefer putting a try/except block to import CCL here rather than the dynamic import in ccl.py; makes more sense to have the error caught at import time here rather than runtime.

It depends on whether you want access to static properties of CCL class without having pyccl installed (e.g. for auto-installing it from cobaya install scripts - which is why mostly dynamical imports in the Cobaya default likelihoods).

timothydmorton · 2021-04-20T18:00:42Z

soliket/ccl.py


    def initialize(self):
+        self.ccl = __import__("pyccl")


see above regarding importing. I think top-level import of pyccl is fine here.

timothydmorton · 2021-04-20T20:30:36Z

soliket/ccl.py


    def initialize(self):
+        self.ccl = __import__("pyccl")
        self._var_pairs = set()
        self._required_results = {}

    def get_requirements(self):
        # These are currently required to construct a CCL cosmology object.
        # Ultimately CCL should depend only on observable not parameters
        # 'As' could be substituted by sigma8.


Update comment here to remove reference to 'As', since it's no longer there.

timothydmorton · 2021-04-20T20:34:57Z

soliket/ccl.py

+        # Create a CCL cosmology object. Because we are giving it background 
+        # quantities, it should not depend on the cosmology parameters given
+        cosmo = self.ccl.CosmologyCalculator(
+            Omega_c=Omega_c, Omega_b=Omega_b, h=h, sigma8=0.8, n_s=0.96,


I would feel a bit more comfortable with sigma8 and n_s coming from the provider, unless @damonge tells me it's 100% OK not to do so, not only for this current use case, but for plausible future ones.

If you're passing linear power spectra, sigma8 and n_s should be redundant. You're not doing that here, but see my comment below.

timothydmorton · 2021-04-20T20:37:39Z

soliket/ccl.py

+                        'delta_matter:delta_matter': Pk_lin})
+                cosmo._init_growth({'a': a,
+                        'growth_factor': growth,
+                        'growth_rate': fgrowth})


Is this kosher CCL API usage? My understanding from @damonge is that growth and pk should get set as part of the initialization of the CosmologyCalculator. Maybe we can move the cosmo initialization happen later on after the growth, pk stuff is figured out?

Yes, indeed. This should be done when initializing the CosmologyCalculator. Calling any _whatever method is a bad sign :-). Incidentally, you no longer need to pass growth arrays to CCL if you don't want to (no core quantities depend on them anymore).

Ah that's great, I will change that, thanks!

timothydmorton · 2021-04-20T20:41:51Z

soliket/ccl.py

+class Tester(Likelihood):
+    # Cross and auto data
+
+    auto_file: str = 'input/clgg_noiseless.txt'


what are these paths relative to? See some of the other likelihoods for how to package data. Options might be making it installable, or including the data in the package and finding it with resource_filename.

timothydmorton · 2021-04-20T20:42:33Z

soliket/ccl.py

+    #dcl_file: str = "/Users/Pablo/Code/SOLikeT/soliket/data/simulated_ccl/dcls.npy"
+
+    def initialize(self):
+        self.ccl = __import__("pyccl")


see above comment about global pyccl; I think we should stick with that.

timothydmorton · 2021-04-20T20:43:13Z

soliket/ccl.py

+
+    #ell_file: str = "/Users/Pablo/Code/SOLikeT/soliket/data/simulated_ccl/ell.npy"
+    #cl_file: str = "/Users/Pablo/Code/SOLikeT/soliket/data/simulated_ccl/cls.npy"
+    #dcl_file: str = "/Users/Pablo/Code/SOLikeT/soliket/data/simulated_ccl/dcls.npy"


remove unused code, here and elsewhere

timothydmorton · 2021-04-20T20:45:31Z

soliket/ccl.py

+        sigma = np.concatenate([self.cl_auto_err, self.cl_cross_err])
+        chi2 = delta**2/sigma**2.
+        #chi2 = np.dot(r, self.invcov.dot(r))
+        return -0.5 * np.sum(chi2)


Since this is a gaussian likelihood, this likelihood should inherit from GaussianLikelihood, and define the appropriate _get_data, _get_cov, and _get_theory methods.

(see some of the other likelihoods for examples, or ask me for more clarification)

timothydmorton · 2021-04-20T20:51:25Z

soliket/tests/test_ccl.py

+model = get_model(info)
+loglikes, derived = model.loglikes({"b_hydro": 0.3})
+assert loglikes[0] == 0.3
+print('OK')


This test file should contain various functions named test_... that will get picked up and run by pytest. Including to testing this Tester likelihood here, you should also have functions that test the cross-correlation likelihood implemented in ccl.py. Again, see some of the other likelihoods for examples. There should also be an example test_[ ].yaml file that contains the specification of a model for test running purposes (again, see test_*.yaml in this dir)

timothydmorton · 2021-04-20T20:56:42Z

I just turned on the GH actions to run on PRs; I'm hoping it will run when you add more commits here.

damonge · 2021-04-21T08:35:38Z

BTW, I think I sent this to @Pablo-Lemos and @timothydmorton , but just in case it helps, this is a similar implementation of a CCL theory class we've been using as a prototype in DESC: https://github.com/damonge/ShCl_like/blob/master/shcl_like/clccl.py
@cmbant suggested the initial structure for this.

cmbant · 2021-04-21T09:13:10Z

Regarding imports/different versions - presumably depends on whether the CCL Theory class will soon be part of the ccl package, or if there will be separate versions maintained separately for each expt and/or in Cobaya. Probably the most logical place for it is in CCL, so CCL can keep it consistent with any CCL changes; people would then just e.g. theory "pyccl.ccl" to use it directly from the pyccl package.

cmbant · 2021-04-21T09:15:02Z

In any case, likelihoods and their tests should probably be in separate files from general ccl interfaces.

damonge · 2021-04-21T12:02:12Z

I can push for this to be part of CCL soon-ish. First it might be good to identify if the current structure here or in the repo I sent before works well for all purposes. So how about @Pablo-Lemos finishes this implementation and then I/we look into making it part of CCL (possibly generalising things a bit)

Pablo-Lemos · 2021-04-21T14:17:33Z

I can push for this to be part of CCL soon-ish. First it might be good to identify if the current structure here or in the repo I sent before works well for all purposes. So how about @Pablo-Lemos finishes this implementation and then I/we look into making it part of CCL (possibly generalising things a bit)

That sounds great! Happy to work towards making it more general

Pablo-Lemos · 2021-04-21T14:17:56Z

I have tried to address all the points (thanks everyone)

cmbant · 2021-04-21T15:43:10Z

Thanks - perhaps put the likelihood in a separate file, to keep it distinct from more-general and solikeit-independent CCL? Rather than sigma8=0.8, n_s=0.96 it might be safer to fix to some obviously crazy values to make sure they don't get used by mistake (like 0 and -1)?

I think the suggestion about test naming was to put all the test code in a function like "test_cross_correlation_likelihood()". - that's how pytest files are normally arranged if not using classes (rather than in the .py file body)

timothydmorton · 2021-04-21T19:43:24Z

Yes, I agree with @cmbant - I'd put the likelihood in a separate file, add another test_cross_correlation.py test file that explicitly tests that likelihood, and then format your test files in the standard pytest-compatible way as @cmbant mentioned. Also, please make a test_cross_correlation.yaml file that contains specification of an example model using that likelihood, and add 'cross_correlation' to the list of tested likelihoods in test_runs.py.

I also agree that the natural home for the CCL theory is in pyccl, so we should eventually expect that, but happy to have this be a development sandbox for now. @damonge just let us know when it makes sense for y'all to pull this (or the other version) into pyccl, and we can make sure these likelihoods are updated appropriately, and ccl.py removed from here.

Pablo-Lemos · 2021-04-23T17:48:43Z

I have tried, but I am not familiar with pytest at all, so do let me know if it's not right!

Pablo-Lemos · 2021-04-28T13:07:37Z

Hi @timothydmorton , I tried to address all your points but there seem to be some conflicts now. Is there anything I can do to fix them? Or do you have to approve the changes?

cmbant · 2021-04-28T13:29:40Z

You can just make another push to your branch to fix conflicts

Pablo-Lemos · 2021-04-28T15:38:28Z

Did that do it?

timothydmorton · 2021-04-28T17:17:38Z

soliket/cross_correlation.py

+
+class CrossCorrelationLikelihood(GaussianLikelihood):
+    def initialize(self):
+        self.auto_file: str = 'soliket/data/xcorr_simulated/clgg_noiseless.txt'


All these properties should be class-level properties or set in the .yaml file for the likelihood (not set within .initialize()), and paths should be obtained using get_resource_filename, if the data is to be packaged within soliket.

timothydmorton · 2021-04-28T17:21:29Z

soliket/test_cross_correlation.yaml

+    extra_args:
+      num_massive_neutrinos: 0
+  soliket.ccl.CCL: 
+    python_path: /Users/Pablo/Code/SOLikeT/


timothydmorton · 2021-04-28T17:21:43Z

soliket/test_cross_correlation.yaml

+  sigma8: 
+likelihood:
+  soliket.cross_correlation.CrossCorrelationLikelihood: 
+    python_path: /Users/Pablo/Code/SOLikeT/


This should not be here. Shouldn't have to hard-code paths like this anywhere.

timothydmorton · 2021-04-28T17:23:13Z

soliket/tests/test_runs.py

-@pytest.mark.parametrize("lhood", ["mflike", "lensing", "lensing_lite", "multi"])
-def test_evaluate(lhood):
+@pytest.mark.parametrize("lhood", ["mflike", "lensing", "lensing_lite", "multi, cross_correlation"])
+def test_run(lhood):


This should be changed back to test_evaluate

And below, please add cross_correlation to the test_mcmc parameter list as well.

timothydmorton · 2021-04-28T17:33:40Z

You will see now that the CI tests have failed. You will need to ensure that the xcorr test data is included in the package (you may need to add this to the package_data in setup.py if it is not covered by the existing globs, and then refer to them by get_resource_filename as I instructed in the comments.

Pablo-Lemos · 2021-05-04T14:27:06Z

Hi @timothydmorton, thanks for the comments, I think it has all been addressed

timothydmorton · 2021-06-28T15:37:08Z

Nice job @Pablo-Lemos, thanks! Sorry this took so long for me to get to!

* Added CCL CosmologyCalculator functionality * Added CCL CosmologyCalculator functionality * Added notebook for testing * Added ccl likelihood * small changes * Dynamically importing ccl * Changed import back to top level * Renamed Tester likelihood and added data files * Change creation of CCL cosmo object * Change likelihood to use GaussianLikelihood structure * Removed old comments * Separate likelihood file and tests * Moved test file and other small changes * Removed python path * Updated example notebook * Updated assert statement * Changed max_tries * Increased tolerance of assert * fixed bug

Pablo-Lemos added 5 commits February 25, 2021 12:19

Added CCL CosmologyCalculator functionality

a517b33

Added CCL CosmologyCalculator functionality

da03cf7

Added notebook for testing

8c1762c

Added ccl likelihood

135d4b3

small changes

d9060bf

Dynamically importing ccl

151d03b

timothydmorton requested changes Apr 20, 2021

View reviewed changes

Pablo-Lemos added 3 commits April 21, 2021 12:02

Changed import back to top level

e732c3d

Renamed Tester likelihood and added data files

7d6c5da

Change creation of CCL cosmo object

3feb228

Pablo-Lemos added 2 commits April 21, 2021 15:00

Change likelihood to use GaussianLikelihood structure

5c4c83a

Removed old comments

5082354

Separate likelihood file and tests

46f21dd

Merge branch 'master' into ccl

47f8133

timothydmorton requested changes Apr 28, 2021

View reviewed changes

Pablo-Lemos added 3 commits April 30, 2021 17:34

Moved test file and other small changes

3f2551c

Removed python path

204503a

Solved conflict

b26fd57

Pablo-Lemos added 4 commits May 12, 2021 10:03

Updated example notebook

58e950f

Updated assert statement

29b2d02

Changed max_tries

7c87bf3

Increased tolerance of assert

fad0022

cmbant requested a review from timothydmorton May 31, 2021 07:21

fixed bug

e27586a

Pablo-Lemos marked this pull request as ready for review June 21, 2021 18:34

timothydmorton merged commit 0f8784e into simonsobs:master Jun 28, 2021

Work on CCL module for cross-correlations #16

Work on CCL module for cross-correlations #16

Conversation

Pablo-Lemos commented Apr 20, 2021

cmbant commented Apr 20, 2021

Pablo-Lemos commented Apr 20, 2021

timothydmorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothydmorton commented Apr 20, 2021

damonge commented Apr 21, 2021

cmbant commented Apr 21, 2021

cmbant commented Apr 21, 2021

damonge commented Apr 21, 2021

Pablo-Lemos commented Apr 21, 2021

Pablo-Lemos commented Apr 21, 2021

cmbant commented Apr 21, 2021

timothydmorton commented Apr 21, 2021

Pablo-Lemos commented Apr 23, 2021

Pablo-Lemos commented Apr 28, 2021

cmbant commented Apr 28, 2021

Pablo-Lemos commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothydmorton commented Apr 28, 2021

Pablo-Lemos commented May 4, 2021

timothydmorton commented Jun 28, 2021